From 10304f6dc20880022d3fcf185c04397254b62905 Mon Sep 17 00:00:00 2001 From: Erik Date: Fri, 21 Aug 2026 06:11:33 +0200 Subject: [PATCH] fix(chat): announce enchantment expiry; stop double-printing tells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the four reported chat defects. **Only item spells announced their expiry.** ACE splits the two cases: an enchantment expiring on an ITEM arrives as ordinary server chat ("The spell X on Y has expired.") — which is why those were the only ones showing — while one expiring on the PLAYER arrives as GameEventMagicDispelEnchantment carrying no text at all, because retail's client writes that line itself. ClientMagicSystem::NotifyOfEnchantmentRemoval @0x005686C0 is now ported: the spell's own name plus " has expired.", at LogTextType 7 (Magic), including retail's guards (ids >= 0x8000 skipped, a spell missing from the table prints nothing) and its one special case — spell 0x29A gets " penalty" appended so vitae reads "Vitae penalty has expired." Retail's trailing "\n" is deliberately dropped: its scroll appends raw text, AddText is line-based, and keeping it would print a blank line. **Every tell printed twice.** ACE's GameActionTell replies with a GameMessageSystemChat carrying the finished "You tell X, ..." line (ChatMessageType.OutgoingTell), and we ALSO emitted an optimistic local echo. Retail's own send path, Event_TalkDirectByName @0x00577CF4, has no AddTextToScroll beside it — it just transmits and lets the server's reply print. The local echo is removed, which also makes Tell consistent with Say, which has always relied on the server echo. CH3 had this half-right: it removed the legacy-channel echo for precisely this reason, but kept the Tell echo on the stated grounds that "the server never resends" it. That premise was false. Both test comments asserting it are corrected rather than deleted, since the wrong claim is what made the bug survive review. Solution builds clean; 14,477 tests pass on the standard hermetic lane filter, 0 failures. Co-Authored-By: Claude Opus 5 --- src/AcDream.Core.Net/GameEventWiring.cs | 46 ++++++++- .../Chat/LiveChatCommandRoute.cs | 18 ++-- .../Net/LiveSessionCommandRouterTests.cs | 23 ++--- .../GameEventWiringTests.cs | 94 +++++++++++++++++++ .../Chat/LiveChatCommandRouteTests.cs | 11 ++- 5 files changed, 164 insertions(+), 28 deletions(-) diff --git a/src/AcDream.Core.Net/GameEventWiring.cs b/src/AcDream.Core.Net/GameEventWiring.cs index a2295608..13afc2be 100644 --- a/src/AcDream.Core.Net/GameEventWiring.cs +++ b/src/AcDream.Core.Net/GameEventWiring.cs @@ -688,14 +688,54 @@ public static class GameEventWiring registrar.Register(GameEventType.MagicDispelEnchantment, e => { var p = GameEvents.ParseMagicDispelEnchantment(e.Payload.Span); - if (p is not null) spellbook.OnEnchantmentRemoved(p.Value.Layer, p.Value.SpellId); + if (p is null) return; + spellbook.OnEnchantmentRemoved(p.Value.Layer, p.Value.SpellId); + NotifyOfEnchantmentRemoval((uint)p.Value.SpellId); }); registrar.Register(GameEventType.MagicDispelMultipleEnchantments, e => { var entries = GameEvents.ParseMagicLayeredSpellList(e.Payload.Span); - if (entries is not null) - spellbook.OnEnchantmentsRemoved(entries.Select(item => ((uint)item.SpellId, (uint)item.Layer))); + if (entries is null) return; + spellbook.OnEnchantmentsRemoved(entries.Select(item => ((uint)item.SpellId, (uint)item.Layer))); + foreach (var entry in entries) + NotifyOfEnchantmentRemoval((uint)entry.SpellId); }); + + // Retail spell id 0x29A, special-cased by name in + // NotifyOfEnchantmentRemoval so the line reads "Vitae penalty has + // expired." rather than "Vitae has expired." + const uint VitaePenaltySpellId = 0x29Au; + + // ClientMagicSystem::NotifyOfEnchantmentRemoval @ 0x005686C0. The + // server sends NO text for an enchantment leaving the PLAYER — it is + // the client that announces it. (Enchantments leaving an ITEM are + // different: those arrive as ordinary server chat, which is why item + // spells were the only ones showing up.) + void NotifyOfEnchantmentRemoval(uint spellId) + { + if (onInterfaceText is null) + return; + + // Retail's own guards, in order: ids at or above 0x8000 are not + // spells and are skipped outright, and a spell missing from the + // table returns without printing. + if (spellId >= 0x8000 + || !spellbook.TryGetMetadata(spellId, out SpellMetadata meta)) + { + return; + } + + // Vitae reads "Vitae penalty has expired." — retail appends the + // word to the spell's own name for this one id. + string name = spellId == VitaePenaltySpellId + ? meta.Name + " penalty" + : meta.Name; + + // AddTextToScroll(text, 7, 1, 0). Retail's literal carries a + // trailing newline because its scroll appends raw text; AddText is + // line-based, so adding one here would print a blank line. + onInterfaceText($"{name} has expired.", RetailLogTextType.Magic); + } registrar.Register(GameEventType.MagicPurgeEnchantments, _ => spellbook.OnPurgeAll()); registrar.Register(GameEventType.MagicPurgeBadEnchantments, diff --git a/src/AcDream.Runtime/Chat/LiveChatCommandRoute.cs b/src/AcDream.Runtime/Chat/LiveChatCommandRoute.cs index 827f3348..b4128b39 100644 --- a/src/AcDream.Runtime/Chat/LiveChatCommandRoute.cs +++ b/src/AcDream.Runtime/Chat/LiveChatCommandRoute.cs @@ -159,16 +159,14 @@ public sealed class LiveChatCommandRoute case ChatChannelKind.Tell: if (string.IsNullOrEmpty(command.TargetName)) return; - if (!SendIfActive(() => - bindings.SendTell(command.TargetName, command.Text))) - { - return; - } - bindings.Chat.OnSelfSent( - ChatKind.Tell, - command.Text, - logTextType: (uint)RetailLogTextType.SpeechDirectSend, - targetOrChannel: command.TargetName); + // No local echo. The server sends the "You tell X, ..." line + // back itself (ACE GameActionTell -> GameMessageSystemChat with + // ChatMessageType.OutgoingTell 0x04), and retail's own send + // path just transmits: Event_TalkDirectByName @ 0x00577CF4 has + // no AddTextToScroll beside it. Echoing optimistically printed + // the line twice. Say above already relies on the server echo + // for exactly this reason. + SendIfActive(() => bindings.SendTell(command.TargetName, command.Text)); return; } diff --git a/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs b/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs index ad8b7818..80a656e2 100644 --- a/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs +++ b/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs @@ -161,17 +161,18 @@ public sealed class LiveSessionCommandRouterTests Assert.Equal([("Friend", "hello")], tells); Assert.Equal([(0x00000800u, "group")], channels); - // CH3 (2026-08-09): Fellowship is one of the ACE server-echoing - // legacy channels (resends with an empty sender) — the router must - // NOT also emit a local optimistic echo, or the line double-prints. - // Only the Tell echo (which the server never resends) survives. - Assert.Collection( - chat.Snapshot(), - entry => - { - Assert.Equal(ChatKind.Tell, entry.Kind); - Assert.Equal("Friend", entry.Sender); - }); + // CH3 (2026-08-09) got the Fellowship half right — ACE resends legacy + // channel lines, so a local echo would double-print — but its stated + // reason for keeping the Tell echo, "which the server never resends", + // was wrong: ACE's GameActionTell replies with a GameMessageSystemChat + // carrying the finished "You tell Friend, ..." line + // (ChatMessageType.OutgoingTell). Tells double-printed in the chat + // window for exactly that reason. Retail's own send path + // @0x00577CF4 has no AddTextToScroll beside it either. + // + // So NEITHER emits a local echo now, and Say has always worked this + // way. + Assert.Empty(chat.Snapshot()); } [Fact] diff --git a/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs b/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs index f093d5d2..8ca7cb69 100644 --- a/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs +++ b/tests/AcDream.Core.Net.Tests/GameEventWiringTests.cs @@ -1,5 +1,6 @@ using System; using System.Buffers.Binary; +using System.Collections.Generic; using System.IO; using System.Text; using AcDream.Core.Chat; @@ -1813,6 +1814,99 @@ public sealed class GameEventWiringTests Assert.Empty(rentPayment); } + /// + /// An enchantment leaving the PLAYER is announced by the client, not the + /// server — so nothing printed at all until this was wired. + /// + /// + /// ACE splits the two cases: an enchantment expiring on an ITEM arrives as + /// ordinary server chat ("The spell X on Y has expired."), which is why + /// item spells were the only ones the player ever saw. One expiring on the + /// player arrives as GameEventMagicDispelEnchantment carrying NO text, + /// because retail's client writes the line itself — + /// ClientMagicSystem::NotifyOfEnchantmentRemoval @0x005686C0, printed at + /// LogTextType 7 (Magic). + /// + [Fact] + public void DispelledEnchantment_AnnouncesItsExpiryAtTheRetailTextType() + { + var lines = new List<(string Text, RetailLogTextType Type)>(); + var dispatcher = new GameEventDispatcher(); + GameEventWiring.WireAll( + dispatcher, + new ClientObjectTable(), + new CombatState(), + SpellbookWithNames(), + new ChatLog(), + onInterfaceText: (text, type) => lines.Add((text, type))); + + dispatcher.Dispatch(GameEventEnvelope.TryParse(WrapEnvelope( + GameEventType.MagicDispelEnchantment, + DispelPayload(spellId: 1234, layer: 1)))!.Value); + + Assert.Equal( + ("Fire Protection Self has expired.", RetailLogTextType.Magic), + Assert.Single(lines)); + } + + [Fact] + public void DispelledVitaeReadsAsAPenalty() + { + // Retail appends " penalty" to this one spell's own name, so the line + // reads "Vitae penalty has expired." rather than "Vitae has expired." + var lines = new List<(string Text, RetailLogTextType Type)>(); + var dispatcher = new GameEventDispatcher(); + GameEventWiring.WireAll( + dispatcher, + new ClientObjectTable(), + new CombatState(), + SpellbookWithNames(), + new ChatLog(), + onInterfaceText: (text, type) => lines.Add((text, type))); + + dispatcher.Dispatch(GameEventEnvelope.TryParse(WrapEnvelope( + GameEventType.MagicDispelEnchantment, + DispelPayload(spellId: 0x29A, layer: 1)))!.Value); + + Assert.Equal("Vitae penalty has expired.", Assert.Single(lines).Text); + } + + [Fact] + public void ADispelledSpellMissingFromTheTablePrintsNothing() + { + // Retail returns without printing when InqSpellBase fails, rather than + // announcing a blank name. + var lines = new List<(string Text, RetailLogTextType Type)>(); + var dispatcher = new GameEventDispatcher(); + GameEventWiring.WireAll( + dispatcher, + new ClientObjectTable(), + new CombatState(), + SpellbookWithNames(), + new ChatLog(), + onInterfaceText: (text, type) => lines.Add((text, type))); + + dispatcher.Dispatch(GameEventEnvelope.TryParse(WrapEnvelope( + GameEventType.MagicDispelEnchantment, + DispelPayload(spellId: 4321, layer: 1)))!.Value); + + Assert.Empty(lines); + } + + private static Spellbook SpellbookWithNames() + => new(SpellTable.LoadFromReader(new System.IO.StringReader( + "Spell ID,Name,Flags [Hex]\n" + + "1234,Fire Protection Self,0x4\n" + + "666,Vitae,0x4\n"))); + + private static byte[] DispelPayload(ushort spellId, ushort layer) + { + byte[] payload = new byte[4]; + BinaryPrimitives.WriteUInt16LittleEndian(payload, spellId); + BinaryPrimitives.WriteUInt16LittleEndian(payload.AsSpan(2), layer); + return payload; + } + private static byte[] BuildEnchantment( ushort spellId, ushort layer, diff --git a/tests/AcDream.Runtime.Tests/Chat/LiveChatCommandRouteTests.cs b/tests/AcDream.Runtime.Tests/Chat/LiveChatCommandRouteTests.cs index 04d8fa79..87e0b61c 100644 --- a/tests/AcDream.Runtime.Tests/Chat/LiveChatCommandRouteTests.cs +++ b/tests/AcDream.Runtime.Tests/Chat/LiveChatCommandRouteTests.cs @@ -47,10 +47,13 @@ public sealed class LiveChatCommandRouteTests "client:QueryAge", ], sent); - ChatEntry echo = Assert.Single(communication.Chat.Snapshot()); - Assert.Equal(ChatKind.Tell, echo.Kind); - Assert.Equal("Bob", echo.Sender); - Assert.Equal("secret", echo.Text); + // A tell is SENT and nothing is echoed locally. The server returns the + // "You tell Bob, ..." line itself (ACE GameActionTell -> + // GameMessageSystemChat with ChatMessageType.OutgoingTell), and retail's + // own send path @0x00577CF4 has no AddTextToScroll beside it — so an + // optimistic echo here printed every tell TWICE in the chat window. + // Say has always relied on the server echo for exactly this reason. + Assert.Empty(communication.Chat.Snapshot()); route.Dispose(); route.Publish(new SendServerCommandCmd("@stale"));