From 0ef33934a2747a29200aabba5caa4a3a37a2573a Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Mon, 17 Aug 2026 23:32:04 +0200 Subject: [PATCH] refactor(messages): plan compact rows like cards instead of assuming a fixed height Compact mode ran an ImGuiListClipper with CompactRowHeight = 18f. Two things were wrong with that. The number: at the default 12.75pt the font is 17px, and ChunkRenderer pushes ItemSpacing to zero for the whole chunk loop, so a single-line compact row advances the cursor by 17, not 18. The clipper seeded the cursor one pixel too low per row, which accumulates into a visible drift against the scrollbar. The assumption: compact rows are not constant height at all. DrawCompactRow renders content with wrap: true, and WrapEncodedLine submits one text item per wrapped line. At 620px and 17px type that kicks in around 70 characters, so most chat lines are multi-line. Both densities now share DrawRows/DrawLinearAndMeasure over the existing CardClipPlanner, with the row painter passed in. The fixed height and the clipper are gone. Also corrects the CompensatedDummy comment: the cached heights carry no trailing ItemSpacing (every row ends inside DrawChunks, where spacing is zero, and ImGui writes the advance at submission). The compensation is correct because it cancels the spacing the dummy itself appends. CardClipPlanStep drives the invalidation hook directly, which now sits behind the settle gate, so it walks a synthetic clock past the window. --- HellionChat/SelfTests/CardClipPlanStep.cs | 18 ++++- HellionChat/Ui/Components/MessageList.cs | 87 ++++++++++------------- 2 files changed, 52 insertions(+), 53 deletions(-) diff --git a/HellionChat/SelfTests/CardClipPlanStep.cs b/HellionChat/SelfTests/CardClipPlanStep.cs index 9442e6e..e90d5fa 100644 --- a/HellionChat/SelfTests/CardClipPlanStep.cs +++ b/HellionChat/SelfTests/CardClipPlanStep.cs @@ -1,6 +1,7 @@ using System.Collections.Generic; using Dalamud.Bindings.ImGui; using Dalamud.Plugin.SelfTest; +using HellionChat.Util; namespace HellionChat.SelfTests; @@ -55,7 +56,10 @@ internal sealed class CardClipPlanStep : ISelfTestStep int remaining; try { - messages.RunHeightCacheInvalidationForSelfTest(tab, 400f); + // v1.10.0/A1: the fingerprint gate waits for the value to settle, so + // the step walks a synthetic clock past the window instead of sleeping. + var clock = Environment.TickCount64; + messages.RunHeightCacheInvalidationForSelfTest(tab, 400f, clock); using (var snap = tab.Messages.GetReadOnly(3)) { if (snap.Count > 0) @@ -64,12 +68,20 @@ internal sealed class CardClipPlanStep : ISelfTestStep Plugin.Config.NameFormMode = savedForm == NameFormMode.Full ? NameFormMode.Initials : NameFormMode.Full; - remaining = messages.RunHeightCacheInvalidationForSelfTest(tab, 400f); + messages.RunHeightCacheInvalidationForSelfTest(tab, 400f, clock); + clock += LayoutFingerprintGate.SettleMs; + remaining = messages.RunHeightCacheInvalidationForSelfTest(tab, 400f, clock); } finally { Plugin.Config.NameFormMode = savedForm; - messages.RunHeightCacheInvalidationForSelfTest(tab, 400f); + var restore = Environment.TickCount64 + LayoutFingerprintGate.SettleMs * 2; + messages.RunHeightCacheInvalidationForSelfTest(tab, 400f, restore); + messages.RunHeightCacheInvalidationForSelfTest( + tab, + 400f, + restore + LayoutFingerprintGate.SettleMs + ); } if (remaining != 0) diff --git a/HellionChat/Ui/Components/MessageList.cs b/HellionChat/Ui/Components/MessageList.cs index b0499b1..9386647 100644 --- a/HellionChat/Ui/Components/MessageList.cs +++ b/HellionChat/Ui/Components/MessageList.cs @@ -8,14 +8,11 @@ using HellionChat.Util; namespace HellionChat.Ui.Components; -// Virtualised message list. Compact mode uses ImGuiListClipper (constant row -// height); card mode rows wrap to arbitrary heights, so it runs its own -// prefix-sum clipper (CardClipPlanner) over a per-message height cache, dropped -// whenever the layout fingerprint (font/density/name-mode/width) changes. +// Virtualised message list. Both densities wrap to arbitrary heights, so both +// run the same prefix-sum clipper (CardClipPlanner) over a per-message height +// cache, dropped whenever the layout fingerprint settles on a new value. internal sealed class MessageList { - private const float CompactRowHeight = 18f; - private readonly FontManager _fonts; private readonly ChunkRenderer _chunkRenderer; @@ -70,10 +67,11 @@ internal sealed class MessageList ) => CardClipPlanner.Plan(heights, scrollY, viewportHeight); // SelfTest hook (B2): drives the live invalidation, returns the tab's remaining - // cached-height count so the step can assert the drop. - internal int RunHeightCacheInvalidationForSelfTest(Tab tab, float contentWidth) + // cached-height count so the step can assert the drop. nowMs is a parameter so + // the step can step past the settle window without sleeping (v1.10.0/A1). + internal int RunHeightCacheInvalidationForSelfTest(Tab tab, float contentWidth, long nowMs) { - InvalidateHeightCacheIfLayoutChanged(tab, contentWidth); + InvalidateHeightCacheIfLayoutChanged(tab, contentWidth, nowMs); using var messages = tab.Messages.GetReadOnly(3); return messages.Count(m => m.Height.ContainsKey(tab.Identifier)); } @@ -99,7 +97,7 @@ internal sealed class MessageList // record compare per frame, a clear only after a real settings/resize change // stopped moving. The gate is what keeps a slider drag from rebuilding the // whole tab on every frame. - private void InvalidateHeightCacheIfLayoutChanged(Tab tab, float contentWidth) + private void InvalidateHeightCacheIfLayoutChanged(Tab tab, float contentWidth, long nowMs) { if (!_fingerprintGates.TryGetValue(tab.Identifier, out var gate)) { @@ -107,7 +105,7 @@ internal sealed class MessageList _fingerprintGates[tab.Identifier] = gate; } - if (!gate.ShouldInvalidate(BuildLayoutFingerprint(contentWidth), Environment.TickCount64)) + if (!gate.ShouldInvalidate(BuildLayoutFingerprint(contentWidth), nowMs)) return; using var messages = tab.Messages.GetReadOnly(3); @@ -131,10 +129,14 @@ internal sealed class MessageList // and a runaway content-height computation. var compact = Plugin.Config.UseCompactDensity; - // B2: drop stale cached heights before the snapshot draw (card path only — - // compact rows are constant height). Width read here while it is valid. - if (!compact) - InvalidateHeightCacheIfLayoutChanged(tab, ImGui.GetContentRegionAvail().X); + // B2: drop stale cached heights before the snapshot draw. Both densities + // need this now -- compact rows are not constant height either, they wrap. + // Width read here while it is valid. + InvalidateHeightCacheIfLayoutChanged( + tab, + ImGui.GetContentRegionAvail().X, + Environment.TickCount64 + ); using var messages = tab.Messages.GetReadOnly(3); @@ -143,10 +145,7 @@ internal sealed class MessageList // the parent child's scroll state, which is the one MainWindow owns. var pinnedToBottom = ImGui.GetScrollY() >= ImGui.GetScrollMaxY() - 1f; - if (compact) - DrawCompact(messages); - else - DrawCard(tab, messages); + DrawRows(tab, messages, compact ? DrawCompactRow : DrawCardRow); // B3-5: scroll values are frame-constant inside the child, so this // reflects the current frame's state wherever it runs; kept after the @@ -213,28 +212,6 @@ internal sealed class MessageList _scrollToBottomRequested = true; } - private void DrawCompact(IReadOnlyList messages) - { - unsafe - { - var clipper = new ImGuiListClipperPtr(ImGuiNative.ImGuiListClipper()); - try - { - clipper.Begin(messages.Count, CompactRowHeight); - while (clipper.Step()) - { - for (var i = clipper.DisplayStart; i < clipper.DisplayEnd; i++) - DrawCompactRow(messages[i]); - } - clipper.End(); - } - finally - { - clipper.Destroy(); - } - } - } - private void DrawCompactRow(Message message) { // B2-1/B2-2: render the sender through DrawChunks (the name-aware path @@ -259,7 +236,9 @@ internal sealed class MessageList _chunkRenderer.DrawChunks(message.Content, wrap: true, handler: _handler, lineWidth: 0f); } - private void DrawCard(Tab tab, IReadOnlyList messages) + // Shared by both densities: compact rows wrap too, so neither has a constant + // height the ImGuiListClipper could work with. drawRow is the only difference. + private void DrawRows(Tab tab, IReadOnlyList messages, Action drawRow) { var tabId = tab.Identifier; var count = messages.Count; @@ -280,15 +259,18 @@ internal sealed class MessageList if (!allCached) { - DrawCardLinearAndMeasure(tabId, messages); + DrawLinearAndMeasure(tabId, messages, drawRow); return; } var plan = CardClipPlanner.Plan(heights, ImGui.GetScrollY(), ImGui.GetWindowSize().Y); - // Cached heights already include one trailing ItemSpacing.y; a dummy adds its - // own, which would shove the first visible row down one spacing (jitter). - // Subtract one spacing per dummy so offset AND total content height stay exact. + // A dummy is submitted outside any style push, so it appends its own + // trailing ItemSpacing.y. Subtracting one spacing per dummy makes the dummy + // advance the cursor by exactly the planned height. (The measured row + // heights carry no trailing spacing: every row ends inside DrawChunks, + // which pushes ItemSpacing to zero, and ImGui writes the advance at item + // submission time.) var spacingY = ImGui.GetStyle().ItemSpacing.Y; float CompensatedDummy(float planned) => Math.Max(0f, planned - spacingY); @@ -307,7 +289,7 @@ internal sealed class MessageList { var msg = messages[i]; var before = ImGui.GetCursorPosY(); - DrawCardRow(msg); + drawRow(msg); var after = ImGui.GetCursorPosY(); msg.Height[tabId] = after - before; msg.IsVisible[tabId] = true; @@ -318,13 +300,18 @@ internal sealed class MessageList } // First-frame / post-invalidation fallback: draw + measure every row into the - // cache so the next frame can take the planned path. - private void DrawCardLinearAndMeasure(Guid tabId, IReadOnlyList messages) + // cache so the next frame can take the planned path. The settle gate on the + // layout fingerprint is what keeps a resize drag from landing here every frame. + private void DrawLinearAndMeasure( + Guid tabId, + IReadOnlyList messages, + Action drawRow + ) { foreach (var msg in messages) { var before = ImGui.GetCursorPosY(); - DrawCardRow(msg); + drawRow(msg); var after = ImGui.GetCursorPosY(); msg.Height[tabId] = after - before; msg.IsVisible[tabId] = ImGui.IsItemVisible();