chore(plugin-ui): review cleanup — hermetic memo tests, shelf button anchors, outline pin, bounded miss cache; file #486/#487; correct #461
- Split the two hermetic RetailMarkupIconResolver memoization tests (and their counting fakes) out of the Lane=InstalledDat class into a new untagged RetailMarkupIconResolverMemoizationTests.cs so CI's portable filter (Lane!=InstalledDat) actually runs them. - PluginSidePanel: move the entry button's Anchors = AnchorEdges.None from the Add() call site into PluginShelfButton's own constructor (same comment carried over) so a second construction path cannot miss it. - UiRectOutlinePainterOrderTests: assert the back panel's border segment carries exactly 4 quads (24 vertices, FloatsPerVertex each) so a partial outline cannot pass the painter-order check. - RetailMarkupIconResolver: document the type as UI-thread-only (every caller is a draw-time icon source) and bound the MISS cache to 256 entries with FIFO eviction — HIT entries stay unbounded (bounded by the DAT's own surface count already). New test proves the 257th distinct miss evicts the first (re-probe count rises); verified failing first against the un-bounded code (Expected 258, Actual 257) before restoring the fix. - docs/plugin-ui-markup.md: split the icon-binding row's failure mode into Build-time (missing property only — the binder never checks CLR type) vs. draw-time (a resolved value that cannot convert to a number throws from the draw, not from Build). - docs/ISSUES.md: filed #486 (credits picture scroll frozen by the per-draw anchor pass) and #487 (radar compass tokens candidate, same mechanism, unconfirmed); corrected #461's causality — the graceful logout/reveal-cancel log lines are printed by LiveSessionController.Tick's catch -> StopAfterFailure -> StopCore AFTER the motion-update exception, then it rethrows, so the logout is a consequence of the crash, not its cause; real chain is the #462 stalled login-reveal materialization leaving PlayerMovementController in RuntimeOwnedDormant outside its SetPosition ground phase when an inbound 0xF74C arrives. - Plan doc: recorded the three fix-round commits' verdicts (all PASS) and the Smoke-plugin cleanup commit SHA in the Review ledger, plus a pointer to the two newly filed issues. Verified: dotnet build AcDream.slnx -c Release (0/0), targeted filter 85/0/0, full App suite 7364 passed / 97 skipped / 36 failed (36 pre-existing InstalledDat/Manual/Linux-only failures, unchanged by name from baseline; net +1 passed test from the new eviction test). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
ece2104189
commit
f2d7562c86
8 changed files with 416 additions and 158 deletions
|
|
@ -1,5 +1,3 @@
|
|||
using System.Collections.ObjectModel;
|
||||
using System.Diagnostics.CodeAnalysis;
|
||||
using System.Numerics;
|
||||
using AcDream.App.Rendering;
|
||||
using AcDream.App.Rendering.Gpu;
|
||||
|
|
@ -10,7 +8,6 @@ using AcDream.Content;
|
|||
using AcDream.Core.Items;
|
||||
using AcDream.Plugin.Abstractions;
|
||||
using DatReaderWriter;
|
||||
using DatReaderWriter.Lib.IO;
|
||||
using DatReaderWriter.Options;
|
||||
using Xunit;
|
||||
|
||||
|
|
@ -144,141 +141,4 @@ public sealed class RetailMarkupIconResolverInstalledDatTests
|
|||
{
|
||||
public IGpuFrame? CurrentFrame => null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Residual round finding N4 (perf): <see cref="RetailMarkupIconResolver.ResolveDid"/>
|
||||
/// used to probe the DAT (two cache misses + two B-tree lookups under
|
||||
/// <c>DatDatabaseWrapper</c>'s database lock) on EVERY call for an
|
||||
/// unresolvable id — including once per frame from a draw-time icon
|
||||
/// source, forever. These two tests use a bare-bones fake
|
||||
/// <see cref="IDatReaderWriter"/>/<see cref="IDatDatabase"/> that counts
|
||||
/// <c>TryGet</c> calls directly, rather than the InstalledDat lane
|
||||
/// above — the property under test is call COUNT, not decode
|
||||
/// correctness, so no real DAT files are needed here.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ResolveDid_RepeatedUnresolvableId_ProbesTheDatExactlyOnce()
|
||||
{
|
||||
var dats = new CountingDatReaderWriter();
|
||||
var device = new RecordingGpuDevice();
|
||||
using var cache = new TextureCache(device, dats);
|
||||
var icons = new IconComposer(dats, cache);
|
||||
var objects = new ClientObjectTable();
|
||||
var resolver = new RetailMarkupIconResolver(dats, cache, icons, objects);
|
||||
|
||||
(uint tex1, int w1, int h1) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
(uint tex2, int w2, int h2) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
(uint tex3, int w3, int h3) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
|
||||
Assert.Equal((0u, 0, 0), (tex1, w1, h1));
|
||||
Assert.Equal((0u, 0, 0), (tex2, w2, h2));
|
||||
Assert.Equal((0u, 0, 0), (tex3, w3, h3));
|
||||
|
||||
// Without memoization this would be 3 (one probe pair per call);
|
||||
// with it, the miss is cached after the first resolve.
|
||||
Assert.Equal(1, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(1, dats.HighRes.TryGetCallCount);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ResolveDid_DifferentIds_ProbeIndependently()
|
||||
{
|
||||
// A per-id cache must not collapse distinct ids into one entry.
|
||||
var dats = new CountingDatReaderWriter();
|
||||
var device = new RecordingGpuDevice();
|
||||
using var cache = new TextureCache(device, dats);
|
||||
var icons = new IconComposer(dats, cache);
|
||||
var objects = new ClientObjectTable();
|
||||
var resolver = new RetailMarkupIconResolver(dats, cache, icons, objects);
|
||||
|
||||
resolver.ResolveDid(0x06000001u);
|
||||
resolver.ResolveDid(0x06000002u);
|
||||
resolver.ResolveDid(0x06000001u);
|
||||
|
||||
Assert.Equal(2, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(2, dats.HighRes.TryGetCallCount);
|
||||
}
|
||||
|
||||
/// <summary>Always misses (<c>TryGet</c> returns <see langword="false"/>),
|
||||
/// counting how many times it was asked.</summary>
|
||||
private sealed class CountingDatDatabase : IDatDatabase
|
||||
{
|
||||
public int TryGetCallCount { get; private set; }
|
||||
|
||||
public DatDatabase Db => throw new NotImplementedException();
|
||||
public int Iteration => 0;
|
||||
|
||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value)
|
||||
where T : IDBObj
|
||||
{
|
||||
TryGetCallCount++;
|
||||
value = default;
|
||||
return false;
|
||||
}
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public void Dispose() { }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Only <see cref="Portal"/>/<see cref="HighRes"/> are exercised by
|
||||
/// <see cref="RetailMarkupIconResolver.ResolveDid"/>'s miss path — every
|
||||
/// other member throws if a future change starts touching it, so this
|
||||
/// fake fails loudly rather than silently returning nonsense.
|
||||
/// </summary>
|
||||
private sealed class CountingDatReaderWriter : IDatReaderWriter
|
||||
{
|
||||
public CountingDatDatabase Portal { get; } = new();
|
||||
public CountingDatDatabase HighRes { get; } = new();
|
||||
|
||||
IDatDatabase IDatReaderWriter.Portal => Portal;
|
||||
IDatDatabase IDatReaderWriter.HighRes => HighRes;
|
||||
|
||||
public string SourceDirectory => string.Empty;
|
||||
public IDatDatabase Cell => throw new NotImplementedException();
|
||||
public ReadOnlyDictionary<uint, IDatDatabase> CellRegions => throw new NotImplementedException();
|
||||
public IDatDatabase Language => throw new NotImplementedException();
|
||||
public IDatDatabase Local => throw new NotImplementedException();
|
||||
public ReadOnlyDictionary<uint, uint> RegionFileMap => throw new NotImplementedException();
|
||||
public int PortalIteration => 0;
|
||||
public int CellIteration => 0;
|
||||
public int HighResIteration => 0;
|
||||
public int LanguageIteration => 0;
|
||||
|
||||
public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(uint regionId, T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public IEnumerable<IDatReaderWriter.IdResolution> ResolveId(uint id) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
[return: MaybeNull]
|
||||
public T Get<T>(uint fileId) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value)
|
||||
where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public void Dispose() { }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,221 @@
|
|||
using System.Collections.ObjectModel;
|
||||
using System.Diagnostics.CodeAnalysis;
|
||||
using AcDream.App.Rendering;
|
||||
using AcDream.App.Rendering.Gpu;
|
||||
using AcDream.App.Tests.Rendering;
|
||||
using AcDream.App.Tests.Rendering.Gpu;
|
||||
using AcDream.App.UI;
|
||||
using AcDream.Content;
|
||||
using AcDream.Core.Items;
|
||||
using DatReaderWriter;
|
||||
using DatReaderWriter.Lib.IO;
|
||||
using DatReaderWriter.Options;
|
||||
using Xunit;
|
||||
|
||||
namespace AcDream.App.Tests.UI;
|
||||
|
||||
/// <summary>
|
||||
/// Hermetic companions to <see cref="RetailMarkupIconResolverInstalledDatTests"/>,
|
||||
/// split into their own untagged (no <c>Lane=InstalledDat</c>) class so CI's
|
||||
/// portable filter (<c>.gitea/workflows/ci.yml</c>, <c>Lane!=InstalledDat</c>)
|
||||
/// actually runs them: unlike the rest of that class, these two never touch a
|
||||
/// real DAT — the property under test is DAT-probe call COUNT via a
|
||||
/// bare-bones fake <see cref="IDatReaderWriter"/>/<see cref="IDatDatabase"/>,
|
||||
/// not decode correctness, so no installed retail DAT directory is required.
|
||||
/// </summary>
|
||||
public sealed class RetailMarkupIconResolverMemoizationTests
|
||||
{
|
||||
/// <summary>
|
||||
/// A Decal-habit "add the block prefix again" mistake applied to an
|
||||
/// already-full DID: <c>0x06000165</c> (Melee Defense's real installed
|
||||
/// icon, <c>SampleData.cs:69</c>) plus another <c>0x06000000</c> lands at
|
||||
/// <c>0x0C000165</c> — a value almost certainly absent from both Portal
|
||||
/// and HighRes.
|
||||
/// </summary>
|
||||
private const uint DecalHabitDoubleNormalizedId = 0x0C000165u;
|
||||
|
||||
/// <summary>
|
||||
/// Residual round finding N4 (perf): <see cref="RetailMarkupIconResolver.ResolveDid"/>
|
||||
/// used to probe the DAT (two cache misses + two B-tree lookups under
|
||||
/// <c>DatDatabaseWrapper</c>'s database lock) on EVERY call for an
|
||||
/// unresolvable id — including once per frame from a draw-time icon
|
||||
/// source, forever. These two tests use a bare-bones fake
|
||||
/// <see cref="IDatReaderWriter"/>/<see cref="IDatDatabase"/> that counts
|
||||
/// <c>TryGet</c> calls directly, rather than the InstalledDat lane
|
||||
/// on <see cref="RetailMarkupIconResolverInstalledDatTests"/> — the
|
||||
/// property under test is call COUNT, not decode correctness, so no real
|
||||
/// DAT files are needed here.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ResolveDid_RepeatedUnresolvableId_ProbesTheDatExactlyOnce()
|
||||
{
|
||||
var dats = new CountingDatReaderWriter();
|
||||
var device = new RecordingGpuDevice();
|
||||
using var cache = new TextureCache(device, dats);
|
||||
var icons = new IconComposer(dats, cache);
|
||||
var objects = new ClientObjectTable();
|
||||
var resolver = new RetailMarkupIconResolver(dats, cache, icons, objects);
|
||||
|
||||
(uint tex1, int w1, int h1) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
(uint tex2, int w2, int h2) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
(uint tex3, int w3, int h3) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
|
||||
|
||||
Assert.Equal((0u, 0, 0), (tex1, w1, h1));
|
||||
Assert.Equal((0u, 0, 0), (tex2, w2, h2));
|
||||
Assert.Equal((0u, 0, 0), (tex3, w3, h3));
|
||||
|
||||
// Without memoization this would be 3 (one probe pair per call);
|
||||
// with it, the miss is cached after the first resolve.
|
||||
Assert.Equal(1, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(1, dats.HighRes.TryGetCallCount);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ResolveDid_DifferentIds_ProbeIndependently()
|
||||
{
|
||||
// A per-id cache must not collapse distinct ids into one entry.
|
||||
var dats = new CountingDatReaderWriter();
|
||||
var device = new RecordingGpuDevice();
|
||||
using var cache = new TextureCache(device, dats);
|
||||
var icons = new IconComposer(dats, cache);
|
||||
var objects = new ClientObjectTable();
|
||||
var resolver = new RetailMarkupIconResolver(dats, cache, icons, objects);
|
||||
|
||||
resolver.ResolveDid(0x06000001u);
|
||||
resolver.ResolveDid(0x06000002u);
|
||||
resolver.ResolveDid(0x06000001u);
|
||||
|
||||
Assert.Equal(2, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(2, dats.HighRes.TryGetCallCount);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="RetailMarkupIconResolver"/>'s MISS cache is bounded
|
||||
/// (<c>MaxCachedMisses</c> = 256, FIFO eviction) so a plugin markup
|
||||
/// binding a different bogus/unresolvable DID every frame cannot grow it
|
||||
/// without bound — unlike HIT entries, which are left uncapped because
|
||||
/// that population is bounded by the DAT's own real surface count. Fills
|
||||
/// the cache with 256 distinct misses, proves the oldest is STILL served
|
||||
/// from cache (no re-probe) right up to the cap, then proves a 257th
|
||||
/// distinct miss evicts it — a repeat of the evicted id must re-probe
|
||||
/// the DAT (the call count rises again) rather than serve a stale hit.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ResolveDid_TwoHundredFiftySeventhDistinctMiss_EvictsTheFirst()
|
||||
{
|
||||
var dats = new CountingDatReaderWriter();
|
||||
var device = new RecordingGpuDevice();
|
||||
using var cache = new TextureCache(device, dats);
|
||||
var icons = new IconComposer(dats, cache);
|
||||
var objects = new ClientObjectTable();
|
||||
var resolver = new RetailMarkupIconResolver(dats, cache, icons, objects);
|
||||
|
||||
// Fill the 256-entry miss cache with distinct ids (starting at 1;
|
||||
// did == 0 short-circuits before ever touching the DAT).
|
||||
for (uint id = 1; id <= 256; id++)
|
||||
resolver.ResolveDid(id);
|
||||
|
||||
Assert.Equal(256, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(256, dats.HighRes.TryGetCallCount);
|
||||
|
||||
// Still cached at the cap: a repeat of the OLDEST id must not re-probe.
|
||||
resolver.ResolveDid(1u);
|
||||
Assert.Equal(256, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(256, dats.HighRes.TryGetCallCount);
|
||||
|
||||
// A 257th DISTINCT miss pushes the cache over its cap, evicting the
|
||||
// oldest entry (id 1).
|
||||
resolver.ResolveDid(257u);
|
||||
Assert.Equal(257, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(257, dats.HighRes.TryGetCallCount);
|
||||
|
||||
// id 1 was evicted: asking again must re-probe the DAT (the call
|
||||
// count rises again), rather than serve the now-gone cache entry.
|
||||
resolver.ResolveDid(1u);
|
||||
Assert.Equal(258, dats.Portal.TryGetCallCount);
|
||||
Assert.Equal(258, dats.HighRes.TryGetCallCount);
|
||||
}
|
||||
|
||||
/// <summary>Always misses (<c>TryGet</c> returns <see langword="false"/>),
|
||||
/// counting how many times it was asked.</summary>
|
||||
private sealed class CountingDatDatabase : IDatDatabase
|
||||
{
|
||||
public int TryGetCallCount { get; private set; }
|
||||
|
||||
public DatDatabase Db => throw new NotImplementedException();
|
||||
public int Iteration => 0;
|
||||
|
||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value)
|
||||
where T : IDBObj
|
||||
{
|
||||
TryGetCallCount++;
|
||||
value = default;
|
||||
return false;
|
||||
}
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public void Dispose() { }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Only <see cref="Portal"/>/<see cref="HighRes"/> are exercised by
|
||||
/// <see cref="RetailMarkupIconResolver.ResolveDid"/>'s miss path — every
|
||||
/// other member throws if a future change starts touching it, so this
|
||||
/// fake fails loudly rather than silently returning nonsense.
|
||||
/// </summary>
|
||||
private sealed class CountingDatReaderWriter : IDatReaderWriter
|
||||
{
|
||||
public CountingDatDatabase Portal { get; } = new();
|
||||
public CountingDatDatabase HighRes { get; } = new();
|
||||
|
||||
IDatDatabase IDatReaderWriter.Portal => Portal;
|
||||
IDatDatabase IDatReaderWriter.HighRes => HighRes;
|
||||
|
||||
public string SourceDirectory => string.Empty;
|
||||
public IDatDatabase Cell => throw new NotImplementedException();
|
||||
public ReadOnlyDictionary<uint, IDatDatabase> CellRegions => throw new NotImplementedException();
|
||||
public IDatDatabase Language => throw new NotImplementedException();
|
||||
public IDatDatabase Local => throw new NotImplementedException();
|
||||
public ReadOnlyDictionary<uint, uint> RegionFileMap => throw new NotImplementedException();
|
||||
public int PortalIteration => 0;
|
||||
public int CellIteration => 0;
|
||||
public int HighResIteration => 0;
|
||||
public int LanguageIteration => 0;
|
||||
|
||||
public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TrySave<T>(uint regionId, T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public IEnumerable<IDatReaderWriter.IdResolution> ResolveId(uint id) =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
[return: MaybeNull]
|
||||
public T Get<T>(uint fileId) where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value)
|
||||
where T : IDBObj =>
|
||||
throw new NotImplementedException();
|
||||
|
||||
public void Dispose() { }
|
||||
}
|
||||
}
|
||||
|
|
@ -103,6 +103,19 @@ public sealed class UiRectOutlinePainterOrderTests
|
|||
"the back panel's border segment must be submitted BEFORE the front sprite's " +
|
||||
"segment so it composites underneath it, matching the actual paint order");
|
||||
|
||||
// UiRenderContext.DrawRectOutline draws exactly 4 sides (top, bottom,
|
||||
// left, right), each one quad (2 triangles = 6 vertices) via
|
||||
// TextRenderer.AppendQuad; consecutive same-texture (None) DrawSprite
|
||||
// calls batch into the ONE segment found above. A partial outline —
|
||||
// e.g. a side silently dropped or clipped away — must not pass by
|
||||
// merely having SOME untextured geometry in the right bucket at the
|
||||
// right order; it must have all four sides' worth of vertices.
|
||||
const int expectedQuads = 4;
|
||||
const int expectedVertices = expectedQuads * 6;
|
||||
Assert.Equal(
|
||||
expectedVertices * TextRenderer.FloatsPerVertex,
|
||||
segs[outlineIndex].Verts.Count);
|
||||
|
||||
// No outline geometry may land in TextRenderer's separate untextured
|
||||
// rect bucket at all: that bucket always flushes AFTER every sprite
|
||||
// segment regardless of submission order, which is exactly the bug —
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue