acdream/tests/AcDream.App.Tests/Rendering/TextureCacheRenderSurfaceSamplerKeyTests.cs
Erik 466272ec55 fix(plugin-ui): Slice B review fixes — no magenta for bad DIDs, contract threshold, integral icon bindings, nearest did art, loud markup errors
Two Opus reviews of commit 8217a349e (Slice B: DAT icons in plugin
markup) found one BLOCKING defect and 14 SHOULD-FIX findings. All 15
fixed here in one commit per the review contract.

BLOCKING (finding 1): an unresolvable did painted a magenta square.
TextureCache.GetOrUploadRenderSurface's 1x1 magenta placeholder for a
missing RenderSurface is load-bearing for authored chrome, but
RetailMarkupIconResolver.ResolveDid only short-circuited did==0, so any
other unresolvable id fell through to that placeholder and got scaled
up by UiMarkupIcon/UiMarkupList/UiSimpleButton -- the classic
resolve(0)-style footgun (claude-memory/feedback_ui_resolve_zero_magenta.md),
just triggered by a missing id instead of a literal 0. Fixed by probing
Portal/HighRes existence via IDatReaderWriter.TryGet<RenderSurface>
BEFORE ever calling GetOrUploadRenderSurface -- that TryGet already
serializes concurrent DAT access internally (DatDatabaseWrapper's own
_databaseLock), the same synchronization IconComposer.TryDecode relies
on, so no additional lock was needed. RetailMarkupIconResolver now
takes IDatReaderWriter + TextureCache directly (RetailUiAssets gained a
TextureCache field, its one construction site in
InteractionRetainedUiComposition.cs updated) instead of the old
resolveSprite delegate, since it also needs the nearest-sampled upload
path for finding 6 below.

Finding 2 -- Smoke panel wiring bugs: its list fed iconkind="spell"
raw art DIDs (PluginSpellInfo.IconId) instead of spell ids, so
IMarkupIconResolver.ResolveSpell composited the wrong (or no) badge
every row. SmokeIconPanel.Binding.SpellIds now yields SpellId (the
printed text still shows IconId alongside). The bare-index demo and the
descriptor both moved from the unverified literal 7735 to 0x165 --
retail's real Melee Defense skill icon (SampleData.cs:64,
0x06000165) -- so the owner's visual gate proves real art, not a guess.
StartVisible flipped true, and a character with no self-buffs known
falls back to spell 1's real catalog entry (or an honest "no spells
known" row with icon 0 if even that fails) rather than fabricating art.

Finding 3 -- PluginIcons.Normalize's threshold was silently rewritten
from the contract's 0x01000000 to 0x06000000 during Slice B. Restored
to 0x01000000; the class/method XML docs now state the number directly
(no cref to the private const); the test table adds 0x02000000 (a value
that only distinguishes the two thresholds) and 0x01000000 itself
(passes through unchanged).

Finding 4 -- an unknown iconkind (e.g. "spel") only threw when a
resolver happened to be wired, because BuildIconSource/
BuildRowIconResolve validated inside their own null-icons early return.
A new ValidateIconKind helper runs UNCONDITIONALLY before that branch,
so a malformed iconkind is a Build-time author error on every host.

Finding 5 -- BindUintLiteralOrBinding required an exact uint property
type, rejecting the int-typed bindings Decal-facing code commonly uses
(MosswartMassacre's HudPictureBox.Image is int end to end). It now
matches BindUint's existing leniency: any property, converted via
Convert.ToUInt32 at read time. BindUintList likewise now accepts
IEnumerable<int> alongside IEnumerable<uint> (unchecked per-element
reinterpret -- icon ids never go negative in practice).

Finding 6 -- TextureCache._renderSurfaceGpuTextures was keyed by id
alone, so whichever caller asked for a given RenderSurface id FIRST won
the sampler for every later caller of the same id -- UiDatFont's glyph
atlases already request nearest:true while ResolveChrome's background
art requests nearest:false, so this was a real, reachable collision,
not hypothetical. Rekeyed to (id, nearest); RetailMarkupIconResolver.
ResolveDid now requests nearest:true (pixel-exact 32x32 icon art);
ResolveChrome is untouched (still nearest:false/linear). Audited every
other _renderSurfaceGpuTextures use site (TryGetValue/set/Dispose
iteration+Clear) plus the separate _nearestUiTextureSources/
_linearUiTwinHandles/_uploadMetadata dictionaries (all keyed by handle
or accounting name, unaffected) -- no other eviction/accounting path
assumed id-only keying.

Finding 7 -- column-reservation semantics, per the DECIDED shape:
MarkupDocument now sets button.IconSource / list.IconIdsSource +
IconResolve ONLY when a resolver (icons parameter) is actually wired --
previously button.IconSource was always assigned (even to an
always-empty func on an icons:null host); combined with this finding's
other half -- UiSimpleButton.OnDraw now reserves its icon column
whenever IconSource is non-null, regardless of a per-frame resolve miss,
so a bound id that goes briefly to 0 no longer slides the caption back
and forth -- would have permanently reserved a blank column on such a
host. UiMarkupList already reserved its column whenever IconIdsSource
was set; no draw-side change needed there.

Finding 8 -- added a with/without-icons comparison test for
UiMarkupList (mirroring the existing UiSimpleButton one): asserts the
row text quad's x is strictly greater with an icon column present, and
the icon quad itself has non-zero width.

Finding 9 -- <icon tooltip=""> (empty string) was still treated as
"has a tooltip" by a bare attribute-presence check, making the icon
swallow clicks with no visible tooltip ever appearing. Now uses
!string.IsNullOrWhiteSpace, matching ApplyCommon's own predicate for
every other element's tooltip.

Finding 10 -- PluginShelfButton.OnDraw drew nothing when a non-zero
descriptor icon id resolved to no texture (a bad Decal index, a DAT id
from a different install), rather than falling back to Initials the
way a zero id already did. Now decides once, on the first draw
(memoized, so Initials' string work never repeats every frame): a
failed resolve permanently switches Text to the initials fallback,
computed and assigned BEFORE base.OnDraw actually paints the caption.

Finding 11 -- MarkupDocument.AddElement's switch had no default arm, so
an unknown or miscased element name (<Icon>, <butotn>) silently
vanished from the built tree instead of failing loudly like every
other malformed-markup case. Added a default arm that throws
FormatException. Ran AcDream.Plugins.MossTank.Tests (337/337,
unchanged) and the full App markup suite to confirm no existing markup
relies on an unknown element.

Finding 12 -- PluginPanelDescriptor.IconSurfaceId's XML doc now states
that a bare Decal index is accepted and normalized, citing
PluginIcons.Normalize.

Finding 13 -- docs/plugin-ui-markup.md: replaced the blanket "wrong
type/missing property throws at Build" sentence with the per-attribute
truth table the review produced (which attributes are silent at
runtime vs. throw at Build, and each one's bound CLR/delegate type);
restated the icon-id boundary as 0x01000000; added the "do NOT add
0x06000000 to the four already-full IconId records" warning (citing
SkillBase._iconID / UIRegion::SetImageByDID @0x004f150e); documented
that 0x-prefixed hex is required (an unprefixed all-digit literal
parses as decimal); noted unknown element names now throw; called out
list colors (0xRRGGBB) vs. color=/background=/border= (#AARRGGBB) as
non-interchangeable grammars; documented the root <panel visible>
binding-only exception; corrected the shelf's collapse toggle glyphs
(</>, not the old doc's arrows) and the 28px collapsed-tab size; added
the IconId record-equality API-v1 note; and called out iconkind as
per-<list> (mixed id spaces need pre-normalized DIDs; the composited
spell badge has no did-space escape hatch) plus the existing
one-text-column LIMITATION being deferred to MossTank.

Coverage added for finding 14: a PluginSidePanelTests case proving the
shelf button normalizes a bare descriptor index before resolving, and a
reflection-based unit on AppAutomationSurface.ProjectWorldObject (its
public callers gate on IsAvailable, which needs a fully connected
session heavier than this mapping needs -- the plan's own documented
fallback) proving PluginWorldObject.IconId carries ClientObject.IconId
through unchanged; PluginInventoryItem.IconId uses the identical
one-line pattern inline in CaptureOwnedItems, reviewed by inspection.

Finding 15: recorded a "Review ledger" section in the plan doc with
both slices' commits, both review verdicts, and the two items
explicitly deferred to the MossTank plugin work (multi-column list,
root literal visible).

Verification: full solution builds green. Targeted filter
(Markup|PluginSidePanel|PluginIcons|AppAutomation|TextureCache|
UiDatFont) passes 131/131, including the two InstalledDat-lane tests
(RetailMarkupIconResolverInstalledDatTests,
AppAutomationSurfaceIconInstalledDatTests) actually resolving against
the real installed DAT, not skipping. AcDream.Plugins.MossTank.Tests
passes 337/337 unchanged. Full AcDream.App.Tests suite: 7351 passed /
97 skipped / 36 failed -- identical failure set/count to the
7334/97/36 baseline (the +17 passes are exactly the new/expanded
tests: 2 new PluginIconsTests.Normalize theory rows, 10 new
MarkupIconTests cases, 2 new PluginSidePanelTests cases, 1 new
AppAutomationSurfaceTests case, and the 2 new standalone test files).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-06 15:43:56 +02:00

190 lines
6.9 KiB
C#

using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using AcDream.App.Rendering;
using AcDream.App.Tests.Rendering.Gpu;
using AcDream.Content;
using DatReaderWriter;
using DatReaderWriter.DBObjs;
using DatReaderWriter.Enums;
using DatReaderWriter.Lib.IO;
using Xunit;
namespace AcDream.App.Tests.Rendering;
/// <summary>
/// Review fix round finding 6 (<c>docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md</c>
/// Slice B): <see cref="TextureCache.GetOrUploadRenderSurface"/> must key its
/// render-surface cache by <c>(id, nearest)</c>, not id alone. Before this
/// fix, a plugin markup <c>&lt;icon did&gt;</c> (nearest:true) resolving the
/// SAME id an authored chrome sprite already resolved (nearest:false, via
/// <c>ResolveChrome</c>) would silently inherit whichever sampler the FIRST
/// caller asked for — <see cref="UiDatFont"/>'s own glyph atlases already
/// call this <c>nearest:true</c> while chrome calls it with no
/// <c>nearest</c> argument at all (defaults false), so the collision is a
/// real, reachable production shape, not a hypothetical.
/// </summary>
public sealed class TextureCacheRenderSurfaceSamplerKeyTests
{
private const uint RenderSurfaceId = 0x06006D59u;
[Fact]
public void SameId_DifferentNearest_ReturnsDistinctHandles_AndBothDisposeCleanly()
{
var device = new RecordingGpuDevice();
device.Clear();
var dats = new FakeRenderSurfaceDats();
dats.Register(new RenderSurface
{
Width = 4,
Height = 4,
Format = PixelFormat.PFID_A8R8G8B8,
SourceData = new byte[4 * 4 * 4],
}, RenderSurfaceId);
var cache = new TextureCache(device, dats);
uint linear = cache.GetOrUploadRenderSurface(
RenderSurfaceId, out _, out _, nearest: false);
uint nearest = cache.GetOrUploadRenderSurface(
RenderSurfaceId, out _, out _, nearest: true);
Assert.NotEqual(0u, linear);
Assert.NotEqual(0u, nearest);
Assert.NotEqual(linear, nearest);
// Repeat requests for the SAME (id, nearest) pair are cached — no
// re-upload, same handle back.
Assert.Equal(
linear,
cache.GetOrUploadRenderSurface(RenderSurfaceId, out _, out _, nearest: false));
Assert.Equal(
nearest,
cache.GetOrUploadRenderSurface(RenderSurfaceId, out _, out _, nearest: true));
Assert.Equal(2, device.CreatedTextures.Count);
cache.Dispose();
// Both textures — not just the first-cached one — were disposed.
Assert.All(device.CreatedTextures, texture => Assert.True(texture.IsDisposed));
}
/// <summary>
/// Minimal synthetic <see cref="IDatReaderWriter"/>, same hermetic-test
/// shape as <c>TerrainAtlasDetailTextureTests.FakeDetailTextureDats</c>,
/// except <see cref="Portal"/> is a real (fake) <see cref="IDatDatabase"/>
/// rather than a throwing stub — <see cref="TextureCache.GetOrUploadRenderSurface"/>
/// calls <c>Portal.TryGet</c>/<c>HighRes.TryGet</c> directly, unlike
/// <c>TryCreateDetailTexture</c>'s <c>Get</c>-only access pattern.
/// </summary>
private sealed class FakeRenderSurfaceDats : IDatReaderWriter
{
private readonly Dictionary<uint, IDBObj> _objects = new();
public FakeRenderSurfaceDats()
{
Portal = new FakeDatDatabase(_objects);
HighRes = new FakeDatDatabase(new Dictionary<uint, IDBObj>());
}
public void Register<T>(T obj, uint id) where T : IDBObj => _objects[id] = obj;
public string SourceDirectory => string.Empty;
public IDatDatabase Portal { get; }
public IDatDatabase Cell => throw new NotSupportedException();
public ReadOnlyDictionary<uint, IDatDatabase> CellRegions { get; } =
new(new Dictionary<uint, IDatDatabase>());
public IDatDatabase HighRes { get; }
public IDatDatabase Language => throw new NotSupportedException();
public IDatDatabase Local => throw new NotSupportedException();
public ReadOnlyDictionary<uint, uint> RegionFileMap { get; } =
new(new Dictionary<uint, uint>());
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)
{
bytesRead = 0;
return false;
}
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
Array.Empty<uint>();
public IEnumerable<IDatReaderWriter.IdResolution> ResolveId(uint id) =>
Array.Empty<IDatReaderWriter.IdResolution>();
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
throw new NotSupportedException();
public bool TrySave<T>(uint regionId, T obj, int iteration = 0) where T : IDBObj =>
throw new NotSupportedException();
[return: MaybeNull]
public T Get<T>(uint fileId) where T : IDBObj =>
_objects.TryGetValue(fileId, out IDBObj? obj) && obj is T typed ? typed : default;
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj
{
if (_objects.TryGetValue(fileId, out IDBObj? obj) && obj is T typed)
{
value = typed;
return true;
}
value = default;
return false;
}
public void Dispose()
{
}
}
private sealed class FakeDatDatabase : IDatDatabase
{
private readonly Dictionary<uint, IDBObj> _objects;
public FakeDatDatabase(Dictionary<uint, IDBObj> objects) => _objects = objects;
public DatDatabase Db => throw new NotSupportedException();
public int Iteration => 0;
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj =>
Array.Empty<uint>();
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj
{
if (_objects.TryGetValue(fileId, out IDBObj? obj) && obj is T typed)
{
value = typed;
return true;
}
value = default;
return false;
}
public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value)
{
value = null;
return false;
}
public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead)
{
bytesRead = 0;
return false;
}
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
throw new NotSupportedException();
public void Dispose()
{
}
}
}