From c2e6de1f05dbaee4c7efed8f57f5a8703cc8b104 Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 07:30:51 +0400 Subject: [PATCH] fix(speed): a mount changes the sprite and nothing else `cond` carries Speed, and Speed is written once at login from the class table and never again. Mounting sets VehicleSpeed, which no packet reads: the player rides at walking pace, and nothing in a log or a packet says so. SpeedCalculationService exists to answer exactly this question and **has no callers at all**. It is also wrong where it matters: PlayerStateComponent declares VehicleSpeed as a plain `byte` and ICharacterEntity widens it to `byte?`, so `VehicleSpeed != null` can never be false. Wired as written it would have returned 0 for everybody on foot. Now it keys off IsVehicled, and the mount and dismount paths call it. The existing test passed only because of that bug: it set VehicleSpeed without IsVehicled, which is a state a mount never produces, and the impossible null check happened to answer it correctly. It now sets both, and a second case covers being on foot with VehicleSpeed at 0. Still missing, and out of scope here: the movement BCards. CalculateSpeed's bonus is a commented-out `bonusSpeed = 0` from upstream, so a speed buff moves nothing. --- .../SpeedCalculationService.cs | 12 +++++----- .../TransformationService.cs | 9 +++++++- .../SpeedCalculationServiceTests.cs | 19 ++++++++++++++++ .../TransformationServiceTests.cs | 22 ++++++++++++++++++- .../SpTransformPacketHandlerTests.cs | 4 +++- 5 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/NosCore.GameObject/Services/SpeedCalculationService/SpeedCalculationService.cs b/src/NosCore.GameObject/Services/SpeedCalculationService/SpeedCalculationService.cs index 6e1237e81..e1cf56dbe 100644 --- a/src/NosCore.GameObject/Services/SpeedCalculationService/SpeedCalculationService.cs +++ b/src/NosCore.GameObject/Services/SpeedCalculationService/SpeedCalculationService.cs @@ -35,14 +35,16 @@ public byte CalculateSpeed(INonPlayableEntity nonPlayableEntity) public byte CalculateSpeed(ICharacterEntity characterEntity) { - var defaultSpeed = speedService.GetSpeed(characterEntity.Class); - if (characterEntity.VehicleSpeed != null) + // IsVehicled and not "VehicleSpeed is not null": the component declares that field as + // a plain byte and the interface widens it to byte?, so the null branch could never + // be taken - the service answered VehicleSpeed always, which is 0 on foot. Nothing + // reported it because nothing called the service at all. + if (characterEntity.IsVehicled) { - return (byte)characterEntity.VehicleSpeed; - + return characterEntity.VehicleSpeed ?? 0; } - return CalculateSpeed(characterEntity, defaultSpeed); + return CalculateSpeed(characterEntity, speedService.GetSpeed(characterEntity.Class)); } } } diff --git a/src/NosCore.GameObject/Services/TransformationService/TransformationService.cs b/src/NosCore.GameObject/Services/TransformationService/TransformationService.cs index e980bd8b9..2d6bc3b58 100644 --- a/src/NosCore.GameObject/Services/TransformationService/TransformationService.cs +++ b/src/NosCore.GameObject/Services/TransformationService/TransformationService.cs @@ -31,7 +31,8 @@ namespace NosCore.GameObject.Services.TransformationService { public class TransformationService(IClock clock, IExperienceService experienceService, IJobExperienceService jobExperienceService, IHeroExperienceService heroExperienceService, ILogger logger, - ILogLanguageLocalizer logLanguage, IOptions worldConfiguration) + ILogLanguageLocalizer logLanguage, IOptions worldConfiguration, + SpeedCalculationService.ISpeedCalculationService speedCalculationService) : ITransformationService { public async Task RemoveSpAsync(ClientSession session) @@ -159,6 +160,11 @@ public async Task ChangeVehicleAsync(ClientSession session, Item item) var character = session.Character; character.IsVehicled = true; character.VehicleSpeed = item.Speed; + + // cond carries Speed, not VehicleSpeed, and Speed is written once at login from + // the class table. Without this line the mount changes the sprite and nothing + // else: the player rides at walking pace, and no packet or log says otherwise. + character.Speed = speedCalculationService.CalculateSpeed(character); character.MorphUpgrade = 0; character.MorphDesign = 0; character.Morph = item.SecondMorph == 0 @@ -201,6 +207,7 @@ public async Task RemoveVehicleAsync(ClientSession session) character.IsVehicled = false; character.VehicleSpeed = 0; + character.Speed = speedCalculationService.CalculateSpeed(character); var mapInstance = character.MapInstance; var condPacket = session.Character.GenerateCond(); diff --git a/test/NosCore.GameObject.Tests/Services/SpeedCalculationService/SpeedCalculationServiceTests.cs b/test/NosCore.GameObject.Tests/Services/SpeedCalculationService/SpeedCalculationServiceTests.cs index e090db199..bb5874a07 100644 --- a/test/NosCore.GameObject.Tests/Services/SpeedCalculationService/SpeedCalculationServiceTests.cs +++ b/test/NosCore.GameObject.Tests/Services/SpeedCalculationService/SpeedCalculationServiceTests.cs @@ -57,12 +57,31 @@ public void VehicleSpeedOverridesDefaultSpeed(int characterClassInt) var charMock = new Mock(); charMock.SetupGet(x => x.Class).Returns(characterClass); + // Both, because a mount sets both. Setting only VehicleSpeed described a state that + // cannot happen, and the old null check happened to answer it correctly. + charMock.SetupGet(x => x.IsVehicled).Returns(true); charMock.SetupGet(x => x.VehicleSpeed).Returns(50); var speed = SpeedCalculationService.CalculateSpeed(charMock.Object); Assert.AreEqual(50, speed); } + // On foot VehicleSpeed is 0, not null - the component declares it as a plain byte. Reading + // it whenever it "is not null" therefore answered 0 for everyone, and it went unnoticed + // because nothing called this service. + [TestMethod] + public void OnFootTheVehicleSpeedIsIgnoredRatherThanReturnedAsZero() + { + SpeedService.Setup(x => x.GetSpeed(CharacterClassType.Archer)).Returns((byte)12); + + var charMock = new Mock(); + charMock.SetupGet(x => x.Class).Returns(CharacterClassType.Archer); + charMock.SetupGet(x => x.IsVehicled).Returns(false); + charMock.SetupGet(x => x.VehicleSpeed).Returns((byte)0); + + Assert.AreEqual(12, SpeedCalculationService.CalculateSpeed(charMock.Object)); + } + [TestMethod] public void DefaultMonsterSpeedIsNpcMonsterSpeed() { diff --git a/test/NosCore.GameObject.Tests/Services/TransformationService/TransformationServiceTests.cs b/test/NosCore.GameObject.Tests/Services/TransformationService/TransformationServiceTests.cs index ef1a356d3..20456b346 100644 --- a/test/NosCore.GameObject.Tests/Services/TransformationService/TransformationServiceTests.cs +++ b/test/NosCore.GameObject.Tests/Services/TransformationService/TransformationServiceTests.cs @@ -4,6 +4,8 @@ // |_|\__|\__/ |___/ \__/\__/|_|_\___| // +using NosCore.GameObject.Ecs.Extensions; +using NosCore.Algorithm.SpeedService; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; using NosCore.Algorithm.ExperienceService; @@ -39,7 +41,8 @@ public async Task SetupAsync() new Mock().Object, Logger, TestHelpers.Instance.LogLanguageLocalizer, - TestHelpers.Instance.WorldConfiguration); + TestHelpers.Instance.WorldConfiguration, + new GameObject.Services.SpeedCalculationService.SpeedCalculationService(new SpeedService())); } [TestMethod] @@ -60,6 +63,7 @@ public async Task ChangingVehicleShouldSetVehicledState() .WhenAsync(ChangingToVehicle) .Then(CharacterShouldBeVehicled) .And(VehicleSpeedShouldBeSet) + .And(TheSpeedTheClientSeesIsTheVehicles) .ExecuteAsync(); } @@ -71,9 +75,25 @@ public async Task RemovingVehicleShouldResetState() .WhenAsync(RemovingVehicle) .Then(CharacterShouldNotBeVehicled) .And(VehicleSpeedShouldBeZero) + .And(TheSpeedTheClientSeesIsBackOnFoot) .ExecuteAsync(); } + // VehicleSpeed alone changes nothing the player can feel: `cond` carries Speed, and Speed + // was written once at login from the class table. Asserting only VehicleSpeed is what let + // a mount that adds no speed look correct. + private void TheSpeedTheClientSeesIsTheVehicles() + { + Assert.AreEqual(20, Session.Character.Speed); + Assert.AreEqual(20, Session.Character.GenerateCond().Speed); + } + + private void TheSpeedTheClientSeesIsBackOnFoot() + { + Assert.AreEqual(new SpeedService().GetSpeed(Session.Character.Class), + Session.Character.Speed); + } + private void CharacterHasSpEquipped() { Session.Character.UseSp = true; diff --git a/test/NosCore.PacketHandlers.Tests/Inventory/SpTransformPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Inventory/SpTransformPacketHandlerTests.cs index cda4df4f8..fbbb55c68 100644 --- a/test/NosCore.PacketHandlers.Tests/Inventory/SpTransformPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Inventory/SpTransformPacketHandlerTests.cs @@ -4,6 +4,7 @@ // |_|\__|\__/ |___/ \__/\__/|_|_\___| // +using NosCore.Algorithm.SpeedService; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; using NosCore.Algorithm.ExperienceService; @@ -47,7 +48,8 @@ public async Task SetupAsync() SpTransformPacketHandler = new SpTransformPacketHandler(TestHelpers.Instance.Clock, new TransformationService(TestHelpers.Instance.Clock, new Mock().Object, new Mock().Object, new Mock().Object, - new Mock>().Object, TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.WorldConfiguration), + new Mock>().Object, TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.WorldConfiguration, + new NosCore.GameObject.Services.SpeedCalculationService.SpeedCalculationService(new SpeedService())), TestHelpers.Instance.GameLanguageLocalizer); }