erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions

fix(vt): list column fix round 6/11 — kill per-frame/per-event allocations

Browse source 
DrawColumns allocated four fresh IReadOnlyList<T>?[] sidecar arrays and
ColumnLayout allocated a fresh (float,float)[] EVERY draw call; worse,
OnEventColumns re-invoked every column's source Func a SECOND time (via
UiMarkupListColumn.RowCount()) just to recompute the same row count
DrawColumns had already materialized, and called ColumnLayout a second
time to recompute the same layout — on every single click/scroll event.

Columns is now a property with a custom setter that (re)sizes seven
instance-field caches to Columns.Count exactly once per assignment:
_cachedTextRows/_cachedColorRows/_cachedCheckRows/_cachedIconRows (the
per-column sidecar arrays), _cachedLayout (the column x/width array),
and _scratchIsAuto/_scratchFixedWidth (ComputeColumnLayout's own working
arrays, previously freshly allocated on every layout computation too).
DrawColumns writes into these caches instead of local arrays and records
_cachedRowCount; ComputeColumnLayout (the renamed, now-instance
ColumnLayout) writes into _cachedLayout in place instead of returning a
new array. OnEventColumns reads _cachedRowCount/_cachedLayout/the
per-column cached row arrays instead of re-invoking anything — this also
completes fix item 3's row-bound guard without a second RowCount() call.
UiMarkupListColumn.RowCount() stays as public API (still asserted by an
existing test) but is no longer called from UiMarkupList internally,
which is the "single row-count definition" the fix round asked for.

This does make OnEvent depend on at least one prior Draw call for
correct row/layout data (mirrors real frame order: draw every frame,
then handle input) — the four hit-test click tests that previously
fired OnEvent with no preceding Draw now draw once first, matching what
Scroll_OffsetIsRespectedBySubsequentHitTests already did.

New test: each column source Func increments its own call counter; one
Draw call invokes each exactly once, and five rounds of subsequent
click events invoke none of them again — shown to fail first (16 calls
instead of 1, from OnEventColumns's old RowCount()-per-column-per-event
re-invocation) before the caching change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-09-06 21:07:40 +02:00
parent 5d760331f6
commit 184f687691
2 changed files with 130 additions and 38 deletions
Download patch file Download diff file Expand all files Collapse all files

111
src/AcDream.App/UI/UiMarkupList.cs
View file

@ -42,7 +42,29 @@ public sealed class UiMarkupList : UiElement
/// Items/IconIds/ItemColors field is then ignored (mutually exclusive by
/// construction: <see cref="MarkupDocument"/> never sets both).
/// </summary>
public IReadOnlyList<UiMarkupListColumn>? Columns { get; set; }
public IReadOnlyList<UiMarkupListColumn>? Columns
{
get => _columns;
set
{
_columns = value;
// Fix round item 6: the per-column sidecar arrays and the
// layout/scratch arrays are sized to Columns.Count exactly ONCE
// here (reset whenever a new Columns list is assigned) rather
// than freshly allocated every Draw/OnEvent call — see
// DrawColumns/ComputeColumnLayout/OnEventColumns below, none of
// which allocate an array of their own any more.
int count = value?.Count ?? 0;
_cachedTextRows = new IReadOnlyList<string>?[count];
_cachedColorRows = new IReadOnlyList<uint>?[count];
_cachedCheckRows = new IReadOnlyList<bool>?[count];
_cachedIconRows = new IReadOnlyList<uint>?[count];
_cachedLayout = new (float x, float w)[count];
_scratchIsAuto = new bool[count];
_scratchFixedWidth = new float[count];
_cachedRowCount = 0;
}
}
public UiDatFont? DatFont { get; set; }
public float RowHeight { get; set; } = 18f;
public float Padding { get; set; } = 3f;
@ -52,6 +74,18 @@ public sealed class UiMarkupList : UiElement
public Vector4 SelectedColor { get; set; } = new(0.28f, 0.23f, 0.08f, 0.95f);
private int _topRow;
private IReadOnlyList<UiMarkupListColumn>? _columns;
// ── Fix round item 6: per-column caches (reused between Draw and OnEvent,
// sized to Columns.Count by the Columns setter above) ────────────────────
private IReadOnlyList<string>?[] _cachedTextRows = Array.Empty<IReadOnlyList<string>?>();
private IReadOnlyList<uint>?[] _cachedColorRows = Array.Empty<IReadOnlyList<uint>?>();
private IReadOnlyList<bool>?[] _cachedCheckRows = Array.Empty<IReadOnlyList<bool>?>();
private IReadOnlyList<uint>?[] _cachedIconRows = Array.Empty<IReadOnlyList<uint>?>();
private (float x, float w)[] _cachedLayout = Array.Empty<(float, float)>();
private bool[] _scratchIsAuto = Array.Empty<bool>();
private float[] _scratchFixedWidth = Array.Empty<float>();
private int _cachedRowCount;
public override bool HandlesClick => true;
@ -192,12 +226,9 @@ public sealed class UiMarkupList : UiElement
/// 100% of the remainder" behavior.
/// </para>
/// </summary>
private static (float x, float w)[] ColumnLayout(
IReadOnlyList<UiMarkupListColumn> columns, float totalWidth)
private void ComputeColumnLayout(IReadOnlyList<UiMarkupListColumn> columns, float totalWidth)
{
int n = columns.Count;
var isAuto = new bool[n];
var fixedWidth = new float[n];
float x = 0f;
float sumFixed = 0f;
int autoCount = 0;
@ -205,7 +236,7 @@ public sealed class UiMarkupList : UiElement
{
bool last = i == n - 1;
bool auto = last || columns[i].IsAutoWidth;
isAuto[i] = auto;
_scratchIsAuto[i] = auto;
if (auto)
{
autoCount++;
@ -213,7 +244,7 @@ public sealed class UiMarkupList : UiElement
}
float avail = MathF.Max(0f, totalWidth - x);
float w = MathF.Min(MathF.Max(0f, columns[i].Width), avail);
fixedWidth[i] = w;
_scratchFixedWidth[i] = w;
x += w;
sumFixed += w;
}
@ -221,12 +252,11 @@ public sealed class UiMarkupList : UiElement
float remaining = MathF.Max(0f, totalWidth - sumFixed);
float share = autoCount > 0 ? MathF.Floor(remaining / autoCount) : 0f;
var layout = new (float x, float w)[n];
float cursor = 0f;
for (int i = 0; i < n; i++)
{
float w;
if (isAuto[i])
if (_scratchIsAuto[i])
{
bool isLast = i == n - 1;
w = isLast
@ -235,24 +265,22 @@ public sealed class UiMarkupList : UiElement
}
else
{
w = fixedWidth[i];
w = _scratchFixedWidth[i];
}
layout[i] = (cursor, w);
_cachedLayout[i] = (cursor, w);
cursor += w;
}
return layout;
}
private void DrawColumns(UiRenderContext context, IReadOnlyList<UiMarkupListColumn> columns)
{
// Materialize every column's row source exactly once for this frame —
// matches the single-column path's ItemsSource()/ItemColorsSource()
// calls above. Only one of the four arrays is populated at a given
// column index (per that column's Kind); the others stay null.
var textRows = new IReadOnlyList<string>?[columns.Count];
var colorRows = new IReadOnlyList<uint>?[columns.Count];
var checkRows = new IReadOnlyList<bool>?[columns.Count];
var iconRows = new IReadOnlyList<uint>?[columns.Count];
// calls above. Only one of the four cached arrays is populated at a
// given column index (per that column's Kind); the others stay null.
// Fix round item 6: these are the SAME instance-field arrays
// OnEventColumns reads (sized to Columns.Count by the Columns
// setter) — a click no longer re-invokes any of these Funcs.
int rowCount = 0;
for (int c = 0; c < columns.Count; c++)
{
@ -260,22 +288,23 @@ public sealed class UiMarkupList : UiElement
switch (col.Kind)
{
case UiMarkupListColumnKind.Text:
textRows[c] = col.TextSource!();
colorRows[c] = col.ColorsSource?.Invoke();
rowCount = Math.Max(rowCount, textRows[c]!.Count);
_cachedTextRows[c] = col.TextSource!();
_cachedColorRows[c] = col.ColorsSource?.Invoke();
rowCount = Math.Max(rowCount, _cachedTextRows[c]!.Count);
break;
case UiMarkupListColumnKind.Check:
checkRows[c] = col.CheckSource!();
rowCount = Math.Max(rowCount, checkRows[c]!.Count);
_cachedCheckRows[c] = col.CheckSource!();
rowCount = Math.Max(rowCount, _cachedCheckRows[c]!.Count);
break;
case UiMarkupListColumnKind.Icon:
iconRows[c] = col.IconValuesSource!();
rowCount = Math.Max(rowCount, iconRows[c]!.Count);
_cachedIconRows[c] = col.IconValuesSource!();
rowCount = Math.Max(rowCount, _cachedIconRows[c]!.Count);
break;
}
}
_cachedRowCount = rowCount;
var layout = ColumnLayout(columns, Width);
ComputeColumnLayout(columns, Width);
int visibleRows = VisibleRows;
int selected = SelectedIndexSource();
@ -300,7 +329,7 @@ public sealed class UiMarkupList : UiElement
for (int c = 0; c < columns.Count; c++)
{
(float cellX, float cellW) = layout[c];
(float cellX, float cellW) = _cachedLayout[c];
if (cellW <= 0f)
continue;
@ -317,13 +346,13 @@ public sealed class UiMarkupList : UiElement
switch (columns[c].Kind)
{
case UiMarkupListColumnKind.Text:
DrawTextCell(context, textRows[c], colorRows[c], index, cellX, y);
DrawTextCell(context, _cachedTextRows[c], _cachedColorRows[c], index, cellX, y);
break;
case UiMarkupListColumnKind.Check:
DrawCheckCell(context, checkRows[c], index, cellX, y);
DrawCheckCell(context, _cachedCheckRows[c], index, cellX, y);
break;
case UiMarkupListColumnKind.Icon:
DrawIconCell(context, columns[c], iconRows[c], index, cellX, cellW, y);
DrawIconCell(context, columns[c], _cachedIconRows[c], index, cellX, cellW, y);
break;
}
}
@ -396,9 +425,15 @@ public sealed class UiMarkupList : UiElement
private bool OnEventColumns(in UiEvent e, IReadOnlyList<UiMarkupListColumn> columns)
{
int rowCount = 0;
for (int c = 0; c < columns.Count; c++)
rowCount = Math.Max(rowCount, columns[c].RowCount());
// Fix round item 6: rowCount/layout come from the LAST Draw call's
// materialization (_cachedRowCount/_cachedLayout, populated by
// DrawColumns/ComputeColumnLayout above) — no re-invoking every
// column's source Func or recomputing layout on every event. This
// mirrors normal frame order (draw, then handle input); before the
// first Draw these caches are all zeroed (sized but empty), so an
// event arriving before any Draw is a harmless no-op rather than a
// crash.
int rowCount = _cachedRowCount;
if (e.Type == UiEventType.Scroll)
{
@ -414,11 +449,10 @@ public sealed class UiMarkupList : UiElement
if (row < 0 || row >= VisibleRows || index < 0 || index >= rowCount)
return true; // swallow the press; clicks past the last row do nothing
var layout = ColumnLayout(columns, Width);
float localX = e.Data1;
for (int c = 0; c < columns.Count; c++)
{
(float cellX, float cellW) = layout[c];
(float cellX, float cellW) = _cachedLayout[c];
if (localX < cellX || localX >= cellX + cellW)
continue;
switch (columns[c].Kind)
@ -442,7 +476,8 @@ public sealed class UiMarkupList : UiElement
// click past THIS column's own data must still fire
// nothing (matches the draw side, which already
// skips drawing a cell past its own column's rows).
if (index < columns[c].RowCount())
// Reuses the SAME cached materialization Draw built.
if (index < (_cachedTextRows[c]?.Count ?? 0))
onTextClick(index);
}
else
@ -454,11 +489,11 @@ public sealed class UiMarkupList : UiElement
// A click in a check/icon column fires that column's own
// callback and does NOT change selection. Same
// per-column row-bound guard as the text-onclick case.
if (index < columns[c].RowCount())
if (index < (_cachedCheckRows[c]?.Count ?? 0))
columns[c].CheckChanged?.Invoke(index);
break;
case UiMarkupListColumnKind.Icon:
if (index < columns[c].RowCount())
if (index < (_cachedIconRows[c]?.Count ?? 0))
columns[c].IconClicked?.Invoke(index);
break;
}

57
tests/AcDream.App.Tests/UI/MarkupListColumnsTests.cs
View file

@ -822,6 +822,8 @@ public sealed class MarkupListColumnsTests
public void ClickInTextColumn_SelectsAndDoesNotFireCheckOrIcon()
{
var list = MakeHitTestList(out var sel, out var chk, out var icn);
var (_, ctx) = MakeContext(200f, 200f);
list.DrawSelfAndChildren(ctx); // frame order: draw, then handle input
// Row 1 (y = 20..40), x = 10 (inside the text column [0,20)).
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 10, Data2 = 25 });
@ -835,6 +837,8 @@ public sealed class MarkupListColumnsTests
public void ClickInCheckColumn_FiresCheckCallbackWithRowIndex_AndDoesNotSelect()
{
var list = MakeHitTestList(out var sel, out var chk, out var icn);
var (_, ctx) = MakeContext(200f, 200f);
list.DrawSelfAndChildren(ctx);
// Row 2 (y = 40..60), x = 30 (inside the check column [20,40)).
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 30, Data2 = 45 });
@ -848,6 +852,8 @@ public sealed class MarkupListColumnsTests
public void ClickInIconColumn_FiresIconCallbackWithRowIndex_AndDoesNotSelect()
{
var list = MakeHitTestList(out var sel, out var chk, out var icn);
var (_, ctx) = MakeContext(200f, 200f);
list.DrawSelfAndChildren(ctx);
// Row 0 (y = 0..20), x = 50 (inside the icon column [40,60)).
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 50, Data2 = 5 });
@ -861,6 +867,8 @@ public sealed class MarkupListColumnsTests
public void ClickPastTheLastRow_DoesNothing_ButStillSwallowsThePress()
{
var list = MakeHitTestList(out var sel, out var chk, out var icn, rowCount: 2);
var (_, ctx) = MakeContext(200f, 200f);
list.DrawSelfAndChildren(ctx);
// Only 2 rows (0..40); click at y=100 lands well past the data.
bool handled = list.OnEvent(
@ -954,6 +962,55 @@ public sealed class MarkupListColumnsTests
Assert.Empty(selections);
}
// ── Fix round: kill per-frame/per-event allocations (fix item 6) ────────
[Fact]
public void ClickEvent_DoesNotReinvokeColumnSourceFunctions_ReusesDrawSideMaterialization()
{
// Each column's source Func increments its own counter every time
// it's invoked. One Draw call must invoke each exactly once
// (materializing the frame's rows); any number of SUBSEQUENT click
// events must invoke NONE of them again — the click path reuses the
// draw-side materialized rows/row-count/layout rather than
// re-invoking every source and re-laying-out on every event.
int textCalls = 0, checkCalls = 0, iconCalls = 0;
var list = new UiMarkupList
{
Width = 60f, Height = 200f, RowHeight = 20f,
SelectedIndexSource = () => -1,
Columns = new[]
{
UiMarkupListColumn.Text(20f, () => { textCalls++; return new[] { "a", "b", "c" }; }, null),
UiMarkupListColumn.Check(
20f,
() => { checkCalls++; return new[] { true, false, true }; },
_ => { }),
UiMarkupListColumn.Icon(
20f,
() => { iconCalls++; return new uint[] { 1u, 2u, 3u }; },
id => (id, 16, 16),
_ => { }),
},
};
var (_, ctx) = MakeContext(200f, 200f);
list.DrawSelfAndChildren(ctx);
Assert.Equal(1, textCalls);
Assert.Equal(1, checkCalls);
Assert.Equal(1, iconCalls);
for (int i = 0; i < 5; i++)
{
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 10, Data2 = 25 });
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 30, Data2 = 45 });
list.OnEvent(new UiEvent { Type = UiEventType.MouseDown, Data1 = 50, Data2 = 5 });
}
Assert.Equal(1, textCalls);
Assert.Equal(1, checkCalls);
Assert.Equal(1, iconCalls);
}
[Fact]
public void Scroll_OffsetIsRespectedBySubsequentHitTests()
{