From 3f7821c18d4a2c3f949a3a42e2f32c80b4cb742d Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 25 Apr 2026 20:49:02 +0200 Subject: [PATCH] fix(chat): BuildTell wire field order + retail-style FormatEntry + suppress duplicate Channel echo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-up fixes from the 2026-04-25 live verify session. 1. CRITICAL: BuildTell wire field order. Our outbound layout was [target_name, message] but ACE's GameActionTell.Handle reads [message, target_name] (verified against references/ACE/.../GameActionTell.cs:17-18 verbatim). Result: every /tell since Phase I.3 has been failing with WeenieError 0x052B (CharacterNotAvailable) because ACE was looking up the message text as the recipient name. Swapped the field order in ChatRequests.BuildTell so message is written first; updated the pinned BuildTell test to expect the corrected layout. The WorldSessionChatTests round-trip continues to pass since SendTell delegates to BuildTell. 2. Retail-style FormatEntry. The user asked for the canonical retail strings: /say (own): You say, "text" /say (incoming): Name says, "text" /tell (own echo): You tell Caith, "text" /tell (incoming): Caith tells you, "text" channel: [Trade] +Acdream says, "text" /shout (own): You shout, "text" /shout (incoming):Name shouts, "text" Discriminators: SenderGuid == 0 distinguishes our own outbound echoes (set by OnSelfSent) from real incoming whispers (carry the sender's player guid). Sender == "" or "You" distinguishes our own /say echoes (OnLocalSpeech substitutes "You" when the wire sender is empty per holtburger client/messages.rs:476-487). ChatEntry gains a new ChannelName slot so Channel-kind entries render with the friendly room name ("Trade") instead of "ch 3". Falls back to "ch {ChannelId}" when ChannelName isn't populated (legacy ChatChannel inbound or older callers). 3. Suppress optimistic Channel echo. The user saw duplicates per /trade /lfg in the live trace: [ch 0] Trade: hello <-- our optimistic [ch 3] +Acdream: [Trade] hello <-- ACE's TurbineChat broadcast ACE's TurbineChatHandler at Network/Handlers/TurbineChatHandler.cs broadcasts EventSendToRoom to ALL recipients in the room including the sender, so the canonical echo always arrives via 0xF7DE. Drop the optimistic OnSelfSent for Turbine kinds in GameWindow's SendChatCmd handler; trust the server. Legacy ChatChannel paths (Fellowship / Allegiance / Patron / Monarch / Vassals / CoVassals) keep the optimistic echo because the legacy 0x0147 broadcast may not always come back to the sender. Inbound TurbineChat also stops embedding "[Trade] " into the message text — passes the friendly name out-of-band via the new channelName parameter on ChatLog.OnChannelBroadcast. 11 tests updated for the new format strings (8 in ChatVMTests, 1 in ChatVMCombatTests, 1 BuildTell, plus the format additions cover incoming/outgoing variants per kind). Solution total: 1007 green (243 + 114 + 650), 0 warnings. Tells should now actually deliver. Channel echoes show as [Trade] +Acdream says, "hello" without the duplicate. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/AcDream.App/Rendering/GameWindow.cs | 38 +++++++++--- src/AcDream.Core.Net/Messages/ChatRequests.cs | 25 ++++++-- src/AcDream.Core/Chat/ChatLog.cs | 60 +++++++++++++++++-- .../Panels/Chat/ChatVM.cs | 47 +++++++++++++-- .../Messages/ChatTests.cs | 17 ++++-- .../ChatVMTests.cs | 60 +++++++++++++------ .../Panels/Chat/ChatVMCombatTests.cs | 4 +- 7 files changed, 204 insertions(+), 47 deletions(-) diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index b404dd6..ca47532 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -1258,15 +1258,22 @@ public sealed class GameWindow : IDisposable { if (parsed.Body is AcDream.Core.Net.Messages.TurbineChat.Payload.EventSendToRoom ev) { + // Pass the friendly channel name out-of-band via + // ChatLog.OnChannelBroadcast's channelName param so + // ChatVM.FormatEntry can render the retail-style + // "[Trade] +Acdream says, \"hello\"" without us + // mangling the payload text. string label = TurbineRoomDisplayName(ev.RoomId, ev.ChatType); Chat.OnChannelBroadcast( - channelId: ev.RoomId, - sender: ev.SenderName, - text: $"[{label}] {ev.Message}"); + channelId: ev.RoomId, + sender: ev.SenderName, + text: ev.Message, + channelName: label); } // Response (server ack of an outbound RequestSendToRoomById) - // and Unknown payloads are intentionally not surfaced — the - // user already saw their optimistic local echo. + // and Unknown payloads are intentionally not surfaced — + // the inbound EventSendToRoom for our own message acts as + // the canonical echo. }; // Phase I.3: real ICommandBus. Panels publish SendChatCmd here @@ -1302,6 +1309,14 @@ public sealed class GameWindow : IDisposable // Allegiance is double-routed: try TurbineChat first // (when the player has a Turbine allegiance room) and // fall back to the legacy 0x0147 ChatChannel. + // + // We do NOT optimistic-echo channels: ACE's + // TurbineChatHandler broadcasts EventSendToRoom back + // to the sender too, so we always get the canonical + // echo from the server. Optimistic-echoing here + // double-prints the message (one as "[Trade] hello" + // from us, one as "[Trade] +Acdream says, \"hello\"" + // from the server). Trust the server. var turbine = ResolveTurbineForKind(cmd.Channel, turbineChat); if (turbine is not null) { @@ -1323,9 +1338,10 @@ public sealed class GameWindow : IDisposable senderGuid: senderGuid, text: cmd.Text, cookie: cookie); - chat.OnSelfSent( - AcDream.Core.Chat.ChatKind.Channel, cmd.Text, - targetOrChannel: turbine.Value.DisplayName); + // No optimistic echo: server EventSendToRoom + // broadcast comes back with sender="+Acdream" + // and is rendered by ChatVM as + // "[Trade] +Acdream says, \"hello\"". break; } @@ -1347,6 +1363,12 @@ public sealed class GameWindow : IDisposable $"chat: outbound legacy ChatChannel {resolved.Value.DisplayName} " + $"id=0x{resolved.Value.ChannelId:X8} len={cmd.Text.Length}"); liveSession.SendChannel(resolved.Value.ChannelId, cmd.Text); + // Legacy channels (Fellowship / Allegiance / Patron / + // Monarch / Vassals / CoVassals) — keep the optimistic + // echo because legacy ChatChannel does NOT always + // broadcast back to the sender. ChannelName is the + // friendly display name so ChatVM renders it as + // "[Fellowship] +Acdream says, \"hello\"". chat.OnSelfSent( AcDream.Core.Chat.ChatKind.Channel, cmd.Text, targetOrChannel: resolved.Value.DisplayName); diff --git a/src/AcDream.Core.Net/Messages/ChatRequests.cs b/src/AcDream.Core.Net/Messages/ChatRequests.cs index 485961c..af83d26 100644 --- a/src/AcDream.Core.Net/Messages/ChatRequests.cs +++ b/src/AcDream.Core.Net/Messages/ChatRequests.cs @@ -53,18 +53,35 @@ public static class ChatRequests } /// Send a /tell (whisper) by target character name. + /// + /// Wire field order is message FIRST, then target — that's + /// what ACE's GameActionTell.Handle reads (see + /// references/ACE/Source/ACE.Server/Network/GameAction/Actions/GameActionTell.cs + /// lines 17-18: + /// + /// var message = clientMessage.Payload.ReadString16L(); + /// var target = clientMessage.Payload.ReadString16L(); + /// + /// Filed after a 2026-04-25 live trace where every /tell +Je hello + /// failed with WeenieError 0x052B (CharacterNotAvailable). With the + /// previous (target-first) wire shape, ACE was reading our message + /// payload "hello" as the recipient name, looking it up, finding no + /// character "hello" online, and returning the not-available error. + /// Swapping the field order makes /tell <name> <text> + /// actually deliver. + /// public static byte[] BuildTell(uint gameActionSequence, string targetName, string message) { ArgumentNullException.ThrowIfNull(targetName); ArgumentNullException.ThrowIfNull(message); - byte[] name = PackString16L(targetName); byte[] msg = PackString16L(message); - byte[] body = new byte[12 + name.Length + msg.Length]; + byte[] name = PackString16L(targetName); + byte[] body = new byte[12 + msg.Length + name.Length]; BinaryPrimitives.WriteUInt32LittleEndian(body, GameActionEnvelope); BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(4), gameActionSequence); BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(8), TellOpcode); - Array.Copy(name, 0, body, 12, name.Length); - Array.Copy(msg, 0, body, 12 + name.Length, msg.Length); + Array.Copy(msg, 0, body, 12, msg.Length); + Array.Copy(name, 0, body, 12 + msg.Length, name.Length); return body; } diff --git a/src/AcDream.Core/Chat/ChatLog.cs b/src/AcDream.Core/Chat/ChatLog.cs index c2e9aaa..63e4334 100644 --- a/src/AcDream.Core/Chat/ChatLog.cs +++ b/src/AcDream.Core/Chat/ChatLog.cs @@ -128,15 +128,26 @@ public sealed class ChatLog ChannelId: errorId)); } - /// GameEvent ChannelBroadcast (0x0147). - public void OnChannelBroadcast(uint channelId, string sender, string text) + /// + /// Channel broadcast — legacy ChatChannel (0x0147) or the + /// TurbineChat (0xF7DE) global community channels (General, + /// Trade, LFG, Roleplay, Society, Olthoi). Pass + /// when the caller knows the + /// friendly room name (TurbineChat dispatch always does); the + /// ChatVM formatter renders entries as + /// "[ChannelName] Sender says, \"text\"" when set. + /// + public void OnChannelBroadcast(uint channelId, string sender, string text, string channelName = "") { Append(new ChatEntry( Kind: ChatKind.Channel, Sender: sender, Text: text, SenderGuid: 0, - ChannelId: channelId)); + ChannelId: channelId) + { + ChannelName = channelName, + }); } /// GameEvent Tell (0x02BD) — whisper received. @@ -196,15 +207,43 @@ public sealed class ChatLog }); } - /// Echo the player's own outbound message after local send. + /// + /// Echo the player's own outbound message after local send. + /// + /// + /// + /// Say: pass as + /// empty; the formatter renders "You say, \"text\"". + /// Tell: pass the target name; the formatter + /// renders "You tell {target}, \"text\"". + /// Channel: do not call for global community + /// channels — the server (ACE TurbineChatHandler) echoes the + /// broadcast back to the sender already, so optimistic-echoing + /// would double-print. For legacy channels (Fellowship, + /// Allegiance) where the server may not echo, pass the + /// channel name as ; it + /// becomes the entry's ChannelName. + /// + /// SenderGuid == 0 on the resulting entry is the + /// discriminator the formatter uses to render outgoing-vs-incoming + /// (a real incoming Tell carries the sender's player guid). + /// public void OnSelfSent(ChatKind kind, string text, string targetOrChannel = "") { Append(new ChatEntry( Kind: kind, - Sender: targetOrChannel, // used as "to whom" for Tell / channel name for Channel + // For Tell, Sender carries the target name (so the formatter + // can render "You tell {Sender}..."). For LocalSpeech, we + // leave Sender empty and let the formatter substitute "You". + // For Channel callers, Sender stays empty too — ChannelName + // (below) carries the friendly name. + Sender: kind == ChatKind.Tell ? targetOrChannel : "", Text: text, SenderGuid: 0, - ChannelId: 0)); + ChannelId: 0) + { + ChannelName = kind == ChatKind.Channel ? targetOrChannel : "", + }); } private void Append(ChatEntry entry) @@ -256,4 +295,13 @@ public readonly record struct ChatEntry( /// 's TextColored color choice. /// public Combat.CombatLineKind? CombatKind { get; init; } + + /// + /// Friendly name of the channel for + /// entries (e.g. "General", "Trade", "LFG", "Fellowship"). Empty + /// for non-Channel kinds. Used by ChatVM.FormatEntry to + /// render lines as "[ChannelName] Sender says, \"text\"". + /// Falls back to "ch {ChannelId}" if not populated. + /// + public string ChannelName { get; init; } = ""; } diff --git a/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs b/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs index 14758e4..3365868 100644 --- a/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs +++ b/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs @@ -99,10 +99,28 @@ public sealed class ChatVM /// public static string FormatEntry(ChatEntry entry) => entry.Kind switch { - ChatKind.LocalSpeech => $"{entry.Sender}: {entry.Text}", - ChatKind.RangedSpeech => $"{entry.Sender} says distantly: {entry.Text}", - ChatKind.Channel => $"[ch {entry.ChannelId}] {entry.Sender}: {entry.Text}", - ChatKind.Tell => $"[Tell] {entry.Sender}: {entry.Text}", + // Retail style: "Name says, \"text\"" (incoming) / + // "You say, \"text\"" (own echo). Sender is "" for an + // OnSelfSent echo; OnLocalSpeech substitutes "You" when the + // server sends an empty sender (own-shout echoes). Both forms + // collapse to the singular "You say" verb here. + ChatKind.LocalSpeech => IsOwnSpeaker(entry.Sender) + ? $"You say, \"{entry.Text}\"" + : $"{entry.Sender} says, \"{entry.Text}\"", + ChatKind.RangedSpeech => IsOwnSpeaker(entry.Sender) + ? $"You shout, \"{entry.Text}\"" + : $"{entry.Sender} shouts, \"{entry.Text}\"", + // Channel: "[ChannelName] Sender says, \"text\"". ChannelName + // is populated by callers that know the friendly name (the + // TurbineChat inbound dispatch and OnSelfSent for Channel + // kinds); falls back to "ch {ChannelId}" if not set. + ChatKind.Channel => $"[{ChannelLabel(entry)}] {entry.Sender} says, \"{entry.Text}\"", + // Tell: SenderGuid != 0 means an incoming whisper; == 0 is the + // OnSelfSent echo where Sender carries the target name. Retail + // wording: "You tell Caith, \"hi\"" / "Caith tells you, \"hi\"". + ChatKind.Tell => entry.SenderGuid != 0 + ? $"{entry.Sender} tells you, \"{entry.Text}\"" + : $"You tell {entry.Sender}, \"{entry.Text}\"", ChatKind.System => $"[System] {entry.Text}", ChatKind.Popup => $"[Popup] {entry.Text}", // Phase I.5: emote rendering matches retail's leading-asterisk @@ -121,6 +139,27 @@ public sealed class ChatVM _ => entry.Text, }; + /// + /// True when a chat entry's Sender denotes the local player + /// — i.e. the entry came from OnSelfSent (empty sender) or + /// OnLocalSpeech with the empty-sender substitution kicked + /// in (sender == "You"). Used by the formatter to pick the + /// singular "You say" verb over the third-person "Name says". + /// + private static bool IsOwnSpeaker(string sender) => + string.IsNullOrEmpty(sender) || sender == "You"; + + /// + /// Friendly channel label for a Channel entry. Prefers the entry's + /// ChannelName (set by callers that know the room name) + /// and falls back to "ch {ChannelId}" so legacy paths still + /// produce a readable line. + /// + private static string ChannelLabel(ChatEntry entry) => + string.IsNullOrEmpty(entry.ChannelName) + ? $"ch {entry.ChannelId}" + : entry.ChannelName; + /// /// Phase I.7: snapshot of the chat tail with kind metadata so /// can pick the right rendering primitive diff --git a/tests/AcDream.Core.Net.Tests/Messages/ChatTests.cs b/tests/AcDream.Core.Net.Tests/Messages/ChatTests.cs index ad47adc..02bef46 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/ChatTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/ChatTests.cs @@ -35,8 +35,13 @@ public sealed class ChatTests } [Fact] - public void BuildTell_IncludesBothStrings() + public void BuildTell_WritesMessageFirstThenTarget() { + // Wire order is message-then-target — ACE GameActionTell.Handle + // reads `var message = ...; var target = ...;` in that sequence. + // The previous (target-first) layout caused a 2026-04-25 live + // bug where every /tell failed with WeenieError 0x052B because + // ACE was looking up the message text as the recipient name. byte[] body = ChatRequests.BuildTell( gameActionSequence: 5, targetName: "Alice", message: "hey"); @@ -45,14 +50,14 @@ public sealed class ChatTests int pos = 12; ushort len1 = BinaryPrimitives.ReadUInt16LittleEndian(body.AsSpan(pos)); - Assert.Equal(5, len1); - Assert.Equal("Alice", Encoding.ASCII.GetString(body.AsSpan(pos + 2, 5))); + Assert.Equal(3, len1); + Assert.Equal("hey", Encoding.ASCII.GetString(body.AsSpan(pos + 2, 3))); - // "Alice" record = 2+5=7, pad 1 → advance by 8. + // "hey" record = 2+3=5, pad 3 → advance by 8. pos += 8; ushort len2 = BinaryPrimitives.ReadUInt16LittleEndian(body.AsSpan(pos)); - Assert.Equal(3, len2); - Assert.Equal("hey", Encoding.ASCII.GetString(body.AsSpan(pos + 2, 3))); + Assert.Equal(5, len2); + Assert.Equal("Alice", Encoding.ASCII.GetString(body.AsSpan(pos + 2, 5))); } [Fact] diff --git a/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs b/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs index 199ab94..418ca4e 100644 --- a/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs +++ b/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs @@ -25,8 +25,8 @@ public sealed class ChatVMTests var lines = vm.RecentLines(); Assert.Equal(2, lines.Count); - Assert.Equal("Caith: hello", lines[0]); - Assert.Equal("Regal: world", lines[1]); + Assert.Equal("Caith says, \"hello\"", lines[0]); + Assert.Equal("Regal says, \"world\"", lines[1]); } [Fact] @@ -41,36 +41,62 @@ public sealed class ChatVMTests // Tail = msg25..msg29 (5 entries, oldest first). Assert.Equal(5, lines.Count); - Assert.Equal("A: msg25", lines[0]); - Assert.Equal("A: msg29", lines[4]); + Assert.Equal("A says, \"msg25\"", lines[0]); + Assert.Equal("A says, \"msg29\"", lines[4]); } [Fact] - public void FormatEntry_LocalSpeech_SenderColonText() + public void FormatEntry_LocalSpeech_RetailStyleSays() { - var entry = new ChatEntry(ChatKind.LocalSpeech, "Caith", "hello", 0x5000_0001u, 0); - Assert.Equal("Caith: hello", ChatVM.FormatEntry(entry)); + // Retail format: "Name says, \"text\"" for someone else; + // "You say, \"text\"" for our own /say (sender == "" or "You"). + var incoming = new ChatEntry(ChatKind.LocalSpeech, "Caith", "hello", 0x5000_0001u, 0); + Assert.Equal("Caith says, \"hello\"", ChatVM.FormatEntry(incoming)); + + var ownEcho = new ChatEntry(ChatKind.LocalSpeech, "", "hi there", 0, 0); + Assert.Equal("You say, \"hi there\"", ChatVM.FormatEntry(ownEcho)); + + var ownEchoSubst = new ChatEntry(ChatKind.LocalSpeech, "You", "shouted echo", 0, 0); + Assert.Equal("You say, \"shouted echo\"", ChatVM.FormatEntry(ownEchoSubst)); } [Fact] - public void FormatEntry_RangedSpeech_IncludesDistanceHint() + public void FormatEntry_RangedSpeech_RetailStyleShouts() { - var entry = new ChatEntry(ChatKind.RangedSpeech, "Caith", "hello", 0x5000_0001u, 0); - Assert.Equal("Caith says distantly: hello", ChatVM.FormatEntry(entry)); + var incoming = new ChatEntry(ChatKind.RangedSpeech, "Caith", "hello", 0x5000_0001u, 0); + Assert.Equal("Caith shouts, \"hello\"", ChatVM.FormatEntry(incoming)); + + var ownEcho = new ChatEntry(ChatKind.RangedSpeech, "You", "loud", 0, 0); + Assert.Equal("You shout, \"loud\"", ChatVM.FormatEntry(ownEcho)); } [Fact] - public void FormatEntry_Channel_IncludesChannelId() + public void FormatEntry_Channel_UsesChannelNameWhenPresent() { - var entry = new ChatEntry(ChatKind.Channel, "Caith", "g'day", 0x5000_0001u, 7u); - Assert.Equal("[ch 7] Caith: g'day", ChatVM.FormatEntry(entry)); + // Friendly name takes precedence — "[Trade] Caith says, \"...\"" + var named = new ChatEntry(ChatKind.Channel, "Caith", "g'day", 0x5000_0001u, 7u) + { + ChannelName = "Trade", + }; + Assert.Equal("[Trade] Caith says, \"g'day\"", ChatVM.FormatEntry(named)); + + // Falls back to "ch {id}" when ChannelName isn't set (legacy + // path / older callers). + var unnamed = new ChatEntry(ChatKind.Channel, "Caith", "g'day", 0x5000_0001u, 7u); + Assert.Equal("[ch 7] Caith says, \"g'day\"", ChatVM.FormatEntry(unnamed)); } [Fact] - public void FormatEntry_Tell_PrefixedWithTellTag() + public void FormatEntry_Tell_RetailStyleTells() { - var entry = new ChatEntry(ChatKind.Tell, "Regal", "psst", 0x5000_0002u, 0); - Assert.Equal("[Tell] Regal: psst", ChatVM.FormatEntry(entry)); + // SenderGuid != 0 -> incoming whisper -> "Regal tells you, ..." + var incoming = new ChatEntry(ChatKind.Tell, "Regal", "psst", 0x5000_0002u, 0); + Assert.Equal("Regal tells you, \"psst\"", ChatVM.FormatEntry(incoming)); + + // SenderGuid == 0 -> our own outbound echo -> Sender carries + // the target name -> "You tell Regal, ..." + var ownEcho = new ChatEntry(ChatKind.Tell, "Regal", "psst", 0, 0); + Assert.Equal("You tell Regal, \"psst\"", ChatVM.FormatEntry(ownEcho)); } [Fact] @@ -113,6 +139,6 @@ public sealed class ChatVMTests log.OnLocalSpeech("Caith", "hello", 0x5000_0001u, false); var after = vm.RecentLines(); Assert.Single(after); - Assert.Equal("Caith: hello", after[0]); + Assert.Equal("Caith says, \"hello\"", after[0]); } } diff --git a/tests/AcDream.UI.Abstractions.Tests/Panels/Chat/ChatVMCombatTests.cs b/tests/AcDream.UI.Abstractions.Tests/Panels/Chat/ChatVMCombatTests.cs index fdf9c75..6e8f996 100644 --- a/tests/AcDream.UI.Abstractions.Tests/Panels/Chat/ChatVMCombatTests.cs +++ b/tests/AcDream.UI.Abstractions.Tests/Panels/Chat/ChatVMCombatTests.cs @@ -53,7 +53,7 @@ public sealed class ChatVMCombatTests var line = Assert.Single(vm.RecentLinesDetailed()); Assert.Equal(ChatKind.LocalSpeech, line.Kind); Assert.Null(line.CombatKind); - Assert.Equal("Alice: hi", line.Text); + Assert.Equal("Alice says, \"hi\"", line.Text); } [Fact] @@ -73,7 +73,7 @@ public sealed class ChatVMCombatTests // Plain LocalSpeech entry → Text; combat entry → TextColored. Assert.Contains(renderer.Calls, c => - c.Method == "Text" && (string?)c.Args[0] == "Alice: hi"); + c.Method == "Text" && (string?)c.Args[0] == "Alice says, \"hi\""); var coloredCall = Assert.Single( renderer.Calls, c => c.Method == "TextColored");