diff --git a/src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs b/src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs index bcd0b6e64..9a66e5fb2 100644 --- a/src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs +++ b/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, 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.ForgetUnlearnableSkillsAsync(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 diff --git a/src/NosCore.GameObject/Messaging/Handlers/Nrun/ChangeClassHandler.cs b/src/NosCore.GameObject/Messaging/Handlers/Nrun/ChangeClassHandler.cs index 8400336af..b6afc5a72 100644 --- a/src/NosCore.GameObject/Messaging/Handlers/Nrun/ChangeClassHandler.cs +++ b/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); } } } diff --git a/src/NosCore.GameObject/Services/SkillService/ISkillService.cs b/src/NosCore.GameObject/Services/SkillService/ISkillService.cs index 8a1d25f9b..dd0a456d1 100644 --- a/src/NosCore.GameObject/Services/SkillService/ISkillService.cs +++ b/src/NosCore.GameObject/Services/SkillService/ISkillService.cs @@ -8,5 +8,15 @@ public interface ISkillService Task LoadSkill(ICharacterEntity character); Task LearnClassSkillsAsync(ICharacterEntity character); + + /// + /// Deletes the skills the character cannot learn right now - wrong class, or a job level + /// they no longer have - from memory and from the database both. + /// + /// + /// 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. + /// + Task ForgetUnlearnableSkillsAsync(ICharacterEntity character); } } diff --git a/src/NosCore.GameObject/Services/SkillService/SkillService.cs b/src/NosCore.GameObject/Services/SkillService/SkillService.cs index faa56f4db..962bc9cf8 100644 --- a/src/NosCore.GameObject/Services/SkillService/SkillService.cs +++ b/src/NosCore.GameObject/Services/SkillService/SkillService.cs @@ -18,6 +18,11 @@ public class SkillService(IDao 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 ForgetUnlearnableSkillsAsync(character).ConfigureAwait(false); + var characterSkills = characterSkillDao.Where(x => x.CharacterId == character.VisualId).Adapt>() ?? new List(); var skillToUse = skills.Where(x => characterSkills.Select(s => s.SkillVNum).Contains(x.SkillVNum)); character.Skills.Clear(); @@ -48,22 +53,97 @@ await character.SendPacketAsync(new SkiPacket }).ConfigureAwait(false); } + /// + /// The Adventurer's skills, listed one by one. + /// + /// They cannot be selected by class like every other one, and that is this table's + /// trap: class 0 does not mean "Adventurer", it is the scrap container where 193 entries + /// end up - the emotes, the stat courses, the shop and rest actions, passives. Filtering + /// by class 0 gave an Adventurer all of them, and the bar filled with icons the + /// client will not cast. + /// + /// The range is 200 to 210 inclusive. 209 is in it: Skill.dat gives it class 0, cast id + /// 16, LevelMinimum 1 and the name Capture - it is the Adventurer's pet catcher, which is + /// why CharNewPacketHandler grants it to every new character and why its comment there + /// talks about u_s 16. Left out, an Adventurer cannot catch anything. + /// + /// 211 and 212 are excluded on purpose: the file calls them "Ultra Super Cheating Skill" + /// and "Admin Cheating Skill". + /// + /// Open, and deliberately not guessed at: 300 to 306 are also class 0, with LevelMinimum + /// 10 to 18 and names like "Strengthen Swing". They look like the upgraded forms of + /// 200-206 and they carry the SAME cast ids, so they cannot simply be added alongside. + /// Nothing in the files says how one replaces the other, so they stay out until it does. + /// + private static readonly short[] AdventurerSkills = + { 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210 }; + + /// The skills this class can hold. + private IEnumerable Learnable(ICharacterEntity character) => + character.Class == CharacterClassType.Adventurer + ? skills.Where(s => AdventurerSkills.Contains(s.SkillVNum)) + : skills.Where(s => s.Class == (byte)character.Class); + + /// + /// 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 wrong weapon: 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. + /// + /// The job level is part of the question and not only the class: a class change puts the + /// job level back to 1, so a row for a skill of the destination class that needs + /// job 20 is just as unusable as one belonging to the class left behind. + /// + public async Task ForgetUnlearnableSkillsAsync(ICharacterEntity character) + { + var keep = Learnable(character) + .Where(skill => skill.LevelMinimum <= character.JobLevel) + .Select(skill => skill.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 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)) diff --git a/src/NosCore.PacketHandlers/Command/ChangeClassPacketHandler.cs b/src/NosCore.PacketHandlers/Command/ChangeClassPacketHandler.cs index b9f95acc4..d95f9a7f4 100644 --- a/src/NosCore.PacketHandlers/Command/ChangeClassPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Command/ChangeClassPacketHandler.cs @@ -28,14 +28,15 @@ namespace NosCore.PacketHandlers.Command public class ChangeClassPacketHandler(IPubSubHub pubSubHub, IOptions worldConfiguration, IExperienceService experienceService, IJobExperienceService jobExperienceService, IHeroExperienceService heroExperienceService, - IItemGenerationService itemProvider) + IItemGenerationService itemProvider, + NosCore.GameObject.Services.SkillService.ISkillService skillService) : PacketHandler, 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; } diff --git a/test/NosCore.GameObject.Tests/Services/SkillService/ClassSkillLearningTests.cs b/test/NosCore.GameObject.Tests/Services/SkillService/ClassSkillLearningTests.cs new file mode 100644 index 000000000..093df1d2d --- /dev/null +++ b/test/NosCore.GameObject.Tests/Services/SkillService/ClassSkillLearningTests.cs @@ -0,0 +1,246 @@ +// __ _ __ __ ___ __ ___ ___ +// | \| |/__\ /' _/ / _//__\| _ \ __| +// | | ' | \/ |`._`.| \_| \/ | 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 RememberingSkillDao _dao = null!; + + private static readonly List 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 = 9 }, + // 209 is the Adventurer's Capture: class 0, cast id 16, LevelMinimum 1 in Skill.dat. + new SkillDto { SkillVNum = 209, Class = 0, LevelMinimum = 1, CastId = 16 }, + // A cheat skill, class 0 like the rest of the bucket. + new SkillDto { SkillVNum = 211, Class = 0, LevelMinimum = 0, CastId = 10 }, + // 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(); + _dao = new RememberingSkillDao(); + _service = new NosCore.GameObject.Services.SkillService.SkillService(_dao, 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); + } + + // 209 is the pet catcher. Skill.dat gives it class 0, cast id 16 and LevelMinimum 1, and + // CharNewPacketHandler hands it to every new character - its comment there says a missing + // one makes `u_s 16` answer `cancel 2`, and 16 is exactly this skill's cast id. + // + // The first version of this list left it out, and the test asserted the omission. Losing + // it raises nothing: the Adventurer simply cannot catch anything any more. + [TestMethod] + public async Task TwoHundredAndNineIsTheAdventurersCapture() + { + _session.Character.Class = CharacterClassType.Adventurer; + _session.Character.JobLevel = 20; + + await _service.LearnClassSkillsAsync(_session.Character); + + CollectionAssert.Contains(_session.Character.Skills.Keys.ToList(), (short)209); + } + + // 211 and 212 are what the file calls "Ultra Super Cheating Skill" and "Admin Cheating + // Skill". They sit in class 0 with everything else. + [TestMethod] + public async Task TheCheatSkillsAreNotHandedOut() + { + _session.Character.Class = CharacterClassType.Adventurer; + _session.Character.JobLevel = 20; + + await _service.LearnClassSkillsAsync(_session.Character); + + CollectionAssert.DoesNotContain(_session.Character.Skills.Keys.ToList(), (short)211); + } + + [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); // above the job level + CollectionAssert.DoesNotContain(learned, (short)200); // and nothing from class 0 + } + + // The in-memory dictionary is keyed by skill number and collapses duplicates, so counting + // it proves nothing: the rows are what piled up. This watches the store. + [TestMethod] + public async Task LearningTwiceReusesTheRowInsteadOfAddingOne() + { + _session.Character.Class = CharacterClassType.Swordsman; + _session.Character.JobLevel = 10; + + await _service.LearnClassSkillsAsync(_session.Character); + var rowsAfterFirst = _dao.Rows.Count; + var idOf220 = _dao.Rows.Single(r => r.SkillVNum == 220).Id; + + // Only the persisted side survives a relog, so this is the state the second call + // meets in production. + _session.Character.Skills.Clear(); + await _service.LearnClassSkillsAsync(_session.Character); + + Assert.AreEqual(rowsAfterFirst, _dao.Rows.Count); + Assert.AreEqual(idOf220, _dao.Rows.Single(r => r.SkillVNum == 220).Id); + } + + // A class change puts the job level back to 1, so a row for a skill of the destination + // class needing job 50 is as unusable as one of the class left behind. Left in place it + // came back at the next login, past any level check. + [TestMethod] + public async Task ARowAboveTheJobLevelIsForgottenToo() + { + _session.Character.Class = CharacterClassType.Swordsman; + _session.Character.JobLevel = 1; + _dao.Rows.Add(new CharacterSkillDto + { + Id = Guid.NewGuid(), + CharacterId = _session.Character.VisualId, + SkillVNum = 222 + }); + + await _service.ForgetUnlearnableSkillsAsync(_session.Character); + + Assert.IsFalse(_dao.Rows.Any(r => r.SkillVNum == 222)); + } + + /// + /// A store that remembers, which a Mock does not: these tests are about what ends up in + /// the rows, and a DAO that answers nothing to every query cannot show it. + /// + private sealed class RememberingSkillDao : IDao + { + public List Rows { get; } = new(); + + public Task TryInsertOrUpdateAsync(CharacterSkillDto dto) + { + Rows.RemoveAll(r => r.Id == dto.Id); + Rows.Add(dto); + return Task.FromResult(dto); + } + + public Task TryInsertOrUpdateAsync(IEnumerable dtos) + { + var list = dtos.ToList(); + foreach (var dto in list) + { + Rows.RemoveAll(r => r.Id == dto.Id); + Rows.Add(dto); + } + + return Task.FromResult(list.Count > 0); + } + + public Task FirstOrDefaultAsync( + System.Linq.Expressions.Expression> predicate) => + Task.FromResult(Rows.AsQueryable().FirstOrDefault(predicate)!); + + public IEnumerable? Where( + System.Linq.Expressions.Expression> predicate) => + Rows.AsQueryable().Where(predicate).ToList(); + + public Task TryDeleteAsync(Guid key) + { + var row = Rows.FirstOrDefault(r => r.Id == key); + if (row != null) + { + Rows.Remove(row); + } + + return Task.FromResult(row!); + } + + public Task?> TryDeleteAsync(IEnumerable keys) + { + var removed = new List(); + foreach (var key in keys) + { + var row = Rows.FirstOrDefault(r => r.Id == key); + if (row == null) + { + continue; + } + + Rows.Remove(row); + removed.Add(row); + } + + return Task.FromResult?>(removed); + } + + public IEnumerable LoadAll() => Rows; + } + } +} diff --git a/test/NosCore.PacketHandlers.Tests/Command/ChangeClassPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Command/ChangeClassPacketHandlerTests.cs index 367baf124..bd2db864f 100644 --- a/test/NosCore.PacketHandlers.Tests/Command/ChangeClassPacketHandlerTests.cs +++ b/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().Object); } [TestMethod]