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

fix(plugin-ui): Slice B review fixes — no magenta for bad DIDs, contract threshold, integral icon bindings, nearest did art, loud markup errors

Browse source 
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>
This commit is contained in:
Erik 2026-09-06 15:43:56 +02:00
parent 2ebcc01640
commit 466272ec55
18 changed files with 1124 additions and 135 deletions
Download patch file Download diff file Expand all files Collapse all files

26
docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md
View file

@ -252,3 +252,29 @@ all four places, list icon column aligned with rows.
- **Out of scope (file, do not build):** a `/plugins` chat verb, a plugin
manager panel, icon support in `menu`/`tab`, per-plugin shelves, item icon
drag from plugin panels.
## Review ledger
- Slice A commits: `01b98ca30` (feat: movable, collapsible plugin shelf),
`4fada238e` (review fixes: real grip/toggle children, dialog z-order,
persisted collapse/hide intent), `718005b21` (residuals: padding-drag pin,
live dock detection, single clamper, chord text, register row), `2ebcc0164`
(grip sized from the DAT font, collapsed tab findable).
- Slice B commit: `8217a349e` (feat: DAT icons in plugin markup — icon
element, button/list icons, plugin icon ids).
- Slice A review verdict: two independent Opus lenses (architecture;
plugin-author/retail) found the shelf's shape sound; fix round landed the
three commits above and closed clean.
- Slice B review verdict: two independent Opus lenses found 15 findings (1
BLOCKING — an unresolvable `did` painted a scaled-up magenta placeholder;
14 SHOULD-FIX spanning the normalize threshold, per-attribute binding
leniency, sampler-keying, column-reservation semantics, and doc accuracy).
All 15 fixed in one review fix round (this commit); see its message for
the per-finding breakdown.
- **Deferred to the MossTank plugin work** (explicitly out of scope for this
fix round, carried forward rather than built here):
- Multi-column `<list>` — today's list is one text column plus the
optional Slice B icon column; real tabular rows are MossTank's problem.
- Root `<panel visible>` binding-only literal support — the root element
currently accepts only `{Binding}` for `visible`, not a literal
`visible="true"/"false"` the way every child element does.

137
docs/plugin-ui-markup.md
View file

@ -54,15 +54,38 @@ Every attribute that isn't a plain literal is either:
never touches `UiElement` objects directly, and never from a thread other
than the one that calls `Tick`.
A binding that resolves to the wrong CLR type, or names a property that
doesn't exist, throws `FormatException` **at `Build`** — the same moment any
other malformed attribute throws — never silently at draw time. A resolved
binding that returns an out-of-range or default value (0, empty, null) at
*runtime* draws nothing/looks empty; it never throws after the panel has
loaded.
A binding failure's severity is per-attribute, not one blanket rule — see the
table below. An unrecognized `{Prop}` that resolves loudly (any row marked
"Throws") throws `FormatException` **at `Build`**, the same moment any other
malformed attribute throws, never silently at draw time. The attributes
marked "Silent" instead fall back to something visible-but-harmless at
*runtime* (the literal text, `null`, or `0`) — a plugin author who typos one
of those sees a wrong-looking value on screen rather than a crash, so double
check those four against the markup by eye.
| Attribute(s) | On a missing/mistyped `{Prop}` | Bound CLR type |
|---|---|---|
| `label text`, `field text`, `menu selected`, `tooltip` (any element) | **Silent** — `BindString` falls back to the literal attribute text itself (a typo'd `{Typo}` renders as the literal string `{Typo}`) | `string` (via `.ToString()`) |
| `meter cur`, `meter max` | **Silent** — `BindUint` returns `null` (the meter shows no cur/max) | `uint?` (accepts any integral type) |
| `meter fill`, `slider value` | **Silent** — `BindFloat` returns `0` | `float?`/`float` |
| `list items`, `menu items` | Throws | `IEnumerable<string>` |
| `list colors` | **Silent** if omitted (no color override); throws if present but mistyped | `IEnumerable<uint>` |
| `list icons` (Slice B) | Throws if present but mistyped; omitting it entirely means no icon column at all | `IEnumerable<uint>` **or** `IEnumerable<int>` |
| `<icon>`/`<button icon>` `did`/`spell`/`item` bindings (Slice B) | Throws | any integral type (`uint`, `int`, `long`, `ushort`, a nullable of one, …) via `Convert.ToUInt32` |
| `list selected` | Throws (required int reader) | `int` |
| `tab selected`, `toggle checked` | Throws (required bool reader) | `bool` |
| root `panel visible` | Throws (required bool reader; see the root-only note below) | `bool` |
| `onclick` (button/tab/toggle) | Throws | `Action` |
| `slider onchange` | Throws | `Action<float>` |
| `field onchange`, `field onsubmit`, `menu onchange` | Throws | `Action<string>` |
| `list onchange` | Throws | `Action<int>` |
## Elements
Every element name is validated at `Build`: an unknown or miscased tag (e.g.
`<Icon>`, `<butotn>`) throws `FormatException` rather than silently
vanishing from the built tree.
| Element | Purpose | Key attributes |
|---|---|---|
| `panel` (root) | The window itself | `x y w h title resize visible` |
@ -81,7 +104,25 @@ loaded.
Common to every element via `ApplyCommon`: `name`/`id` (a stable control
name), `visible` (literal `true`/`false` or a bound `bool` property),
`enabled` (same rule), and `tooltip` (a literal string or `{Binding}` shown
through retail's own runtime tooltip popup).
through retail's own runtime tooltip popup, empty/whitespace treated as no
tooltip). The root `<panel>` is the one exception: it does **not** go
through `ApplyCommon` (no `name`/`enabled`/`tooltip`), and its `visible`
attribute accepts a `{Binding}` only — a literal `visible="true"` on the
root is not parsed (unlike every child element, where a literal is fine).
**LIMITATION:** `<list>` has exactly one text column (plus the optional
Slice B icon column) — there is no multi-column list yet. A plugin that
needs tabular rows today pads its own fixed-width text (`$"{name,-16}{value,6}"`).
Real multi-column support is deferred to the MossTank plugin work.
`list colors`' values are `0xRRGGBB` (opaque, no alpha channel), while every
`color=`/`background=`/`border=` attribute elsewhere is `#AARRGGBB` (alpha
first) — the two grammars look similar but are not interchangeable.
Every hex literal (`did`, `0x` id bindings, `list colors` entries) requires
the `0x` prefix to parse as hex; an all-digit string with no prefix
(`did="165"`) parses as **decimal**, not hex — `did="165"` and `did="0x165"`
are different ids.
## The icon-id grammar (Slice B)
@ -97,9 +138,16 @@ everywhere an icon id is accepted:
static uint Normalize(uint idOrIndex);
// 0 -> 0 (no icon)
// 7735 -> 0x06001E37 (bare index -> RenderSurface DID)
// 0x00FFFFFF -> 0x06FFFFFF (largest bare index, just below the boundary)
// 0x01000000 -> 0x01000000 (AT the boundary -> already a DID, unchanged)
// 0x06002D14 -> 0x06002D14 (already a DID, unchanged)
```
The boundary is `0x01000000`: any value below it is treated as a bare
Decal-style index and gets the `0x06000000` RenderSurface block prefix added;
any value at or above it (including `0x01000000` itself) is assumed to
already be a resolvable DID and passes through unchanged.
The host applies `Normalize` at **every** `did`-shaped sink: the descriptor's
`IconSurfaceId` (drawn on the plugin shelf button), and every `<icon did>` /
`<button icon>` (`iconkind="did"`) / `<list icons>` (`iconkind="did"`) value —
@ -114,6 +162,24 @@ re-normalized — they are already in DID space, read straight from the
client's SpellTable/SkillTable/object state. `Normalize` only matters at a
markup `did` sink, where a plugin author might type a bare index by hand.
**Do NOT add `0x06000000` to any of the four `IconId` records above by
hand** — they are already full DIDs, not bare indices. `PluginSkillInfo.IconId`
in particular comes straight from `SkillBase.IconId`, and retail's own
`UIRegion::SetImageByDID(SkillBase._iconID)` (`@0x004f150e`) draws that field
directly as a DID with no `+0x06000000` step of its own — adding the block
prefix again would double-normalize it and resolve nothing (see
`src/AcDream.App/UI/Layout/SampleData.cs:64-82` for the real values: Melee
Defense is `0x06000165`, never `7735`/`0x165`, in that field).
**API-v1 note:** `IconId` is a positional/`init` member on each of the four
records above, so it participates in record equality (`Equals`/`GetHashCode`)
along with every other field. Plugin code that compares two
`PluginSpellInfo`/`PluginSkillInfo`/`PluginInventoryItem`/`PluginWorldObject`
values for equality now also compares their `IconId` — harmless for code
built against the new host (both sides fill it identically), but worth
knowing if you see an equality check that used to succeed start failing
against a host that populates `IconId` where an older one left it `0`.
## The three icon sources
Every icon-bearing attribute (`<icon>`'s `did`/`spell`/`item`, `<button icon>`,
@ -165,34 +231,53 @@ shifts right to make room. `text` may be empty for an icon-only button.
```xml
<list x="12" y="100" w="256" h="108"
items="{SpellRows}" icons="{SpellIconIds}" iconkind="spell"
items="{SpellRows}" icons="{SpellIds}" iconkind="spell"
selected="{SelectedIndex}"/>
```
`icons` is an `IEnumerable<uint>` binding parallel to `items` — Decal's
`IconColumn` convention: a leading square column, `RowHeight - 2` pixels wide,
one icon per row. A row past the end of the icons list, or an id that
resolves to nothing, draws no icon for that row (the text still draws, just
without an icon). Omitting `icons` entirely keeps the list exactly as it was
before Slice B (full-width text, no column).
`icons` is an `IEnumerable<uint>` (or `IEnumerable<int>`) binding parallel to
`items` — Decal's `IconColumn` convention: a leading square column,
`RowHeight - 2` pixels wide, one icon per row. A row past the end of the
icons list, or an id that resolves to nothing, draws no icon for that row
(the text still draws, just without an icon). Omitting `icons` entirely
keeps the list exactly as it was before Slice B (full-width text, no
column).
MosswartMassacre-style example — a list column fed straight from
`PluginSpellInfo.IconId`:
`iconkind` is **per-`<list>`**, not per-row: every id in one list's `icons`
binding is resolved the same way (all `did`, all `spell`, or all `item`).
There is no way to mix kinds within a single list. Two consequences:
- If a plugin's data genuinely mixes id spaces (some rows are raw DIDs, some
are spell ids needing a composited badge), it must pre-normalize/pre-resolve
outside the markup and expose ONE consistent `IEnumerable<uint>` of
`did`-space ids — `iconkind="did"` on the list.
- retail's composited spell badge (power-level backing + tint + self/fellow
overlay) can **only** be reached through `iconkind="spell"` with real spell
ids — there is no DID that already IS the composited result, so a list
that wants the badge look has no `did`-space escape hatch.
MosswartMassacre-style example — a list column composited from spell ids,
with the spell's own raw art DID printed alongside the name for comparison
(`AcDream.Plugins.Smoke`'s own proof panel, `SmokeIconPanel.cs`, is exactly
this pattern):
```csharp
public IEnumerable<uint> SpellIconIds =>
host.Automation.Spells.KnownSelfBuffs.Select(s => s.IconId);
// iconkind="spell": the values MUST be spell ids (what ResolveSpell composites
// a badge from), NOT the spell's raw IconId — those are different id spaces.
public IEnumerable<uint> SpellIds =>
host.Automation.Spells.KnownSelfBuffs.Select(s => s.SpellId);
public IEnumerable<string> SpellRows =>
host.Automation.Spells.KnownSelfBuffs.Select(s => s.Name);
host.Automation.Spells.KnownSelfBuffs.Select(
s => $"{s.Name} (icon 0x{s.IconId:X8})");
```
```xml
<list items="{SpellRows}" icons="{SpellIconIds}" iconkind="did" .../>
<list items="{SpellRows}" icons="{SpellIds}" iconkind="spell" .../>
```
(`did`, not `spell`, here — `PluginSpellInfo.IconId` is the spell's own raw art
tile; use `iconkind="spell"` only when the binding hands the host a **spell
id** and wants retail's composited badge instead of the plain art.)
(A list backed by `PluginSpellInfo.IconId` directly — the spell's own raw art
tile, no composited badge — uses `iconkind="did"` instead, with `icons`
yielding `IconId` rather than `SpellId`.)
## The plugin shelf (Slice A)
@ -205,8 +290,10 @@ like every other window.
- **Drag**: a grip strip across its top (three short dashes) is the move
handle. Dragging elsewhere on the shelf (the buttons themselves, the
padding between them) does not move the window.
- **Collapse**: a small `»`/`«` toggle at the grip's right end shrinks the
shelf to just the grip; button entries stay laid out underneath so
- **Collapse**: a small `>`/`<` toggle at the grip's right end (expanded
shows `>`, collapsed shows `<`) shrinks the shelf to a 28px-tall,
button-sized tab (deliberately findable-sized, not a thin sliver) rather
than just the grip band; button entries stay laid out underneath so
expanding is instant. Persists through the same window-state channel as
position/visibility.
- **Hide/show**: `Shift+Ctrl+F1` (retail's plugin-manager chord,

3
src/AcDream.App/Composition/InteractionRetainedUiComposition.cs
View file

@ -721,7 +721,8 @@ internal sealed class RetailInteractionRetainedUiCompositionFactory
defaultFont,
d.DebugFont,
controls,
iconComposer);
iconComposer,
d.TextureCache);
// Review fix round F12 (2026-08-15): constructed ONCE per
// composition and captured by the ResolveText closure below,
// rather than a fresh DatStringResolver per lookup. The

21
src/AcDream.App/Rendering/TextureCache.cs
View file

@ -38,7 +38,21 @@ public sealed class TextureCache
// Direct-RenderSurface caches for UI sprites: 0x06xxxxxx RenderSurface ids
// decoded directly (Portal/HighRes → DecodeRenderSurface), bypassing the
// Surface→SurfaceTexture chain that GetOrUpload uses for world materials.
private readonly Dictionary<uint, GpuUiTextureEntry> _renderSurfaceGpuTextures = new();
//
// Review fix round (Slice B, docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md
// finding 6): keyed by (id, nearest), not id alone. GetOrUploadRenderSurface's
// sampler is chosen at UPLOAD time (UploadUiTexture bakes `nearest` into the
// registered IGpuSampler) and a table slot is one fixed (texture, sampler)
// pair — the same source id legitimately needs BOTH samplers in the same
// session (UiDatFont's glyph atlases already call this nearest:true while
// ResolveChrome's background/border art calls it nearest:false with no
// `nearest` argument at all). An id-only key made whichever caller asked
// FIRST win the sampler for every later caller of the same id; a plugin
// markup <icon did=...>/<button icon>/<list icons> (nearest:true, Slice B)
// could therefore silently inherit a chrome sprite's linear sampler (or
// vice versa) purely by upload order.
private readonly Dictionary<(uint SurfaceId, bool Nearest), GpuUiTextureEntry>
_renderSurfaceGpuTextures = new();
// Campaign LA gate round 2: the OTHER magenta cause GetOrUploadRenderSurface can
// hit — a non-zero id that simply isn't a RenderSurface in either dat (as opposed
@ -231,7 +245,8 @@ public sealed class TextureCache
/// </summary>
public uint GetOrUploadRenderSurface(uint renderSurfaceId, out int width, out int height, bool nearest = false)
{
if (_renderSurfaceGpuTextures.TryGetValue(renderSurfaceId, out GpuUiTextureEntry existing))
var cacheKey = (renderSurfaceId, nearest);
if (_renderSurfaceGpuTextures.TryGetValue(cacheKey, out GpuUiTextureEntry existing))
{
width = existing.Width; height = existing.Height;
return UiTextureTableHandle.FromSlot(existing.Slot);
@ -262,7 +277,7 @@ public sealed class TextureCache
}
GpuUiTextureEntry entry = UploadUiTexture(decoded, nearest, $"ui-rendersurface-0x{renderSurfaceId:X8}");
_renderSurfaceGpuTextures[renderSurfaceId] = entry;
_renderSurfaceGpuTextures[cacheKey] = entry;
width = decoded.Width; height = decoded.Height;
return UiTextureTableHandle.FromSlot(entry.Slot);
}

59
src/AcDream.App/UI/IMarkupIconResolver.cs
View file

@ -1,4 +1,7 @@
using AcDream.App.Rendering;
using AcDream.Content;
using AcDream.Core.Items;
using DatReaderWriter.DBObjs;
namespace AcDream.App.UI;
@ -42,31 +45,65 @@ public interface IMarkupIconResolver
/// <summary>
/// Production <see cref="IMarkupIconResolver"/>. Built once in
/// <see cref="RetailUiRuntime.MountPlugins"/> from
/// <see cref="RetailUiAssets.ResolveSprite"/>, <see cref="RetailUiAssets.Icons"/>
/// (the one <see cref="IconComposer"/> every authored panel shares), and the
/// live <see cref="ClientObjectTable"/> the inventory UI's own icon resolution
/// already borrows (<c>ToolbarRuntimeBindings.Objects</c>/
/// <c>MagicRuntimeBindings.Objects</c> — both <c>d.Inventory.Objects</c>, the
/// same instance) — no second texture cache or object lookup is introduced.
/// <see cref="RetailUiAssets.Dats"/>, <see cref="RetailUiAssets.TextureCache"/>,
/// <see cref="RetailUiAssets.Icons"/> (the one <see cref="IconComposer"/> every
/// authored panel shares), and the live <see cref="ClientObjectTable"/> the
/// inventory UI's own icon resolution already borrows
/// (<c>ToolbarRuntimeBindings.Objects</c>/<c>MagicRuntimeBindings.Objects</c> —
/// both <c>d.Inventory.Objects</c>, the same instance) — no second texture
/// cache or object lookup is introduced.
/// </summary>
public sealed class RetailMarkupIconResolver : IMarkupIconResolver
{
private readonly Func<uint, (uint tex, int w, int h)> _resolveSprite;
private readonly IDatReaderWriter _dats;
private readonly TextureCache _textureCache;
private readonly IconComposer _icons;
private readonly ClientObjectTable _objects;
public RetailMarkupIconResolver(
Func<uint, (uint tex, int w, int h)> resolveSprite,
IDatReaderWriter dats,
TextureCache textureCache,
IconComposer icons,
ClientObjectTable objects)
{
_resolveSprite = resolveSprite ?? throw new ArgumentNullException(nameof(resolveSprite));
_dats = dats ?? throw new ArgumentNullException(nameof(dats));
_textureCache = textureCache ?? throw new ArgumentNullException(nameof(textureCache));
_icons = icons ?? throw new ArgumentNullException(nameof(icons));
_objects = objects ?? throw new ArgumentNullException(nameof(objects));
}
public (uint tex, int w, int h) ResolveDid(uint did) =>
did == 0u ? (0u, 0, 0) : _resolveSprite(did);
/// <summary>
/// Review fix round (Slice B finding 1): probes for the RenderSurface's
/// existence in Portal/HighRes BEFORE ever calling
/// <see cref="TextureCache.GetOrUploadRenderSurface"/> — that method's 1x1
/// magenta placeholder for a missing id is load-bearing for authored
/// chrome (its own doc comment), and a plugin markup id that resolves to
/// nothing must draw NOTHING, never a scaled-up magenta square (see
/// <c>feedback_ui_resolve_zero_magenta.md</c>: guard on the id, never on
/// the resolved handle). <see cref="IDatReaderWriter.Portal"/>/<see cref="IDatReaderWriter.HighRes"/>'s
/// own <c>TryGet</c> already serializes concurrent DAT access internally
/// (<c>DatDatabaseWrapper.TryGet</c>'s <c>_databaseLock</c>) — the same
/// synchronization <see cref="IconComposer.TryDecode"/> relies on for
/// every one of its own DAT reads — so no additional lock is taken here.
/// Requests the NEAREST sampler (Slice B finding 6): plugin icon art is
/// pixel-exact 32x32 DAT art, the same convention every other icon in the
/// client draws with (dat-font glyphs, composited spell/item icons), and
/// <see cref="TextureCache"/> keys its render-surface cache by
/// <c>(id, nearest)</c> so this never collides with a chrome sprite's
/// linear sampling of the same id.
/// </summary>
public (uint tex, int w, int h) ResolveDid(uint did)
{
if (did == 0u)
return (0u, 0, 0);
if (!_dats.Portal.TryGet<RenderSurface>(did, out _)
&& !_dats.HighRes.TryGet<RenderSurface>(did, out _))
{
return (0u, 0, 0);
}
uint tex = _textureCache.GetOrUploadRenderSurface(did, out int w, out int h, nearest: true);
return (tex, w, h);
}
public (uint tex, int w, int h) ResolveSpell(uint spellId)
{

187
src/AcDream.App/UI/MarkupDocument.cs
View file

@ -213,15 +213,30 @@ public static class MarkupDocument
button.BorderColor = Color(
(string?)el.Attribute("border"));
// Slice B: <button icon="..." iconkind="did|spell|item">.
// Review fix round finding 4: the iconkind literal is
// validated here regardless of whether a resolver is
// wired — a typo like iconkind="spel" must throw at
// Build on every host, not only ones with icon support
// turned on. Finding 7: IconSource is only ASSIGNED when
// a resolver exists — UiSimpleButton now reserves its
// icon column whenever IconSource is non-null (see
// UiPanel.cs), so setting it to an always-empty func on
// an icons:null host would permanently reserve a column
// that never draws anything.
string? buttonIcon = (string?)el.Attribute("icon");
if (buttonIcon is not null)
{
Func<uint> buttonIconReader =
BindUintLiteralOrBinding(buttonIcon, binding, "button icon");
button.IconSource = BuildIconSource(
(string?)el.Attribute("iconkind"),
buttonIconReader,
icons);
string? buttonIconKind = (string?)el.Attribute("iconkind");
ValidateIconKind(buttonIconKind);
if (icons is not null)
{
Func<uint> buttonIconReader =
BindUintLiteralOrBinding(buttonIcon, binding, "button icon");
button.IconSource = BuildIconSource(
buttonIconKind,
buttonIconReader,
icons);
}
}
ApplyCommon(button, el, binding);
if (onClick is not null)
@ -259,8 +274,13 @@ public static class MarkupDocument
};
ApplyCommon(icon, el, binding);
// A tooltip needs this element to be a real hit-test
// target — see UiMarkupIcon's own doc comment.
if (el.Attribute("tooltip") is not null)
// target — see UiMarkupIcon's own doc comment. Review fix
// round finding 9: an EMPTY tooltip="" must not swallow
// clicks either — match ApplyCommon's own
// !IsNullOrWhiteSpace predicate rather than a bare
// attribute-presence check.
string? iconTooltip = (string?)el.Attribute("tooltip");
if (!string.IsNullOrWhiteSpace(iconTooltip))
icon.ClickThrough = false;
parent.AddChild(icon);
break;
@ -488,73 +508,126 @@ public static class MarkupDocument
SelectionChanged = listChanged,
};
// Slice B: <list icons="{IconIds}" iconkind="did|spell|item">.
// Same two rules as <button icon> above: iconkind validates
// regardless of resolver wiring (finding 4), and
// IconIdsSource/IconResolve are only set when a resolver
// exists (finding 7) — UiMarkupList already reserves its
// icon column whenever IconIdsSource is non-null.
string? listIcons = (string?)el.Attribute("icons");
if (!string.IsNullOrWhiteSpace(listIcons))
{
list.IconIdsSource = BindUintList(listIcons, binding, "list icons");
list.IconResolve = BuildRowIconResolve(
(string?)el.Attribute("iconkind"), icons);
string? listIconKind = (string?)el.Attribute("iconkind");
ValidateIconKind(listIconKind);
if (icons is not null)
{
list.IconIdsSource = BindUintList(listIcons, binding, "list icons");
list.IconResolve = BuildRowIconResolve(listIconKind, icons);
}
}
ApplyCommon(list, el, binding);
parent.AddChild(list);
break;
default:
// Review fix round finding 11: an unknown or miscased
// element name previously vanished silently (the switch had
// no default arm) — the same "malformed markup throws at
// Build" rule every other element already follows.
throw new FormatException($"unknown element <{el.Name.LocalName}>");
}
}
/// <summary>
/// Builds the zero-argument icon resolver every markup icon sink (the
/// <c>&lt;icon&gt;</c> element and <c>&lt;button icon&gt;</c>) shares:
/// dispatch by <c>iconkind</c> (default <c>"did"</c>) to the matching
/// <see cref="IMarkupIconResolver"/> method, normalizing <c>did</c>
/// through <see cref="PluginIcons.Normalize"/>
/// (spell/item ids are never DAT RenderSurface DIDs, so they never pass
/// through it). Null <paramref name="icons"/> (no resolver wired) always
/// resolves to nothing rather than throwing.
/// Review fix round finding 4: validates an <c>iconkind</c> attribute
/// (default <c>"did"</c>) UNCONDITIONALLY — before either
/// <see cref="BuildIconSource"/> or <see cref="BuildRowIconResolve"/>'s
/// null-resolver early return, so <c>iconkind="spel"</c> throws
/// <see cref="FormatException"/> at <c>Build</c> on every host, even one
/// with no <see cref="IMarkupIconResolver"/> wired at all. A malformed
/// attribute is a Build-time author error regardless of what the host
/// happens to support.
/// </summary>
private static string ValidateIconKind(string? iconKind) =>
(iconKind ?? "did") switch
{
"did" or "spell" or "item" => iconKind ?? "did",
var other => throw new FormatException(
$"unknown iconkind \"{other}\" (expected did, spell, or item)"),
};
/// <summary>
/// Builds the zero-argument icon resolver the <c>&lt;icon&gt;</c> element
/// uses: dispatch by <c>iconkind</c> (default <c>"did"</c>) to the
/// matching <see cref="IMarkupIconResolver"/> method, normalizing
/// <c>did</c> through <see cref="PluginIcons.Normalize"/> (spell/item ids
/// are never DAT RenderSurface DIDs, so they never pass through it).
/// Null <paramref name="icons"/> (no resolver wired) always resolves to
/// nothing rather than throwing — a standalone <c>&lt;icon&gt;</c> draws
/// nothing either way, so there is no column-reservation concern here
/// the way there is for <c>&lt;button icon&gt;</c>/<c>&lt;list icons&gt;</c>
/// (see their own call sites in <see cref="AddElement"/>).
/// </summary>
private static Func<(uint tex, int w, int h)> BuildIconSource(
string? iconKind, Func<uint> idReader, IMarkupIconResolver? icons)
{
string kind = ValidateIconKind(iconKind);
if (icons is null)
return static () => (0u, 0, 0);
return (iconKind ?? "did") switch
return kind switch
{
"did" => () => icons.ResolveDid(
PluginIcons.Normalize(idReader())),
"spell" => () => icons.ResolveSpell(idReader()),
"item" => () => icons.ResolveItem(idReader()),
var other => throw new FormatException(
$"unknown iconkind \"{other}\" (expected did, spell, or item)"),
_ => throw new InvalidOperationException(
"unreachable — ValidateIconKind already rejected anything else"),
};
}
/// <summary>
/// Same dispatch as <see cref="BuildIconSource"/>, shaped for
/// <c>&lt;list icons&gt;</c>'s per-row resolve (the row's own icon id is
/// the argument rather than a captured reader).
/// the argument rather than a captured reader). Callers only invoke this
/// after confirming <paramref name="icons"/> is non-null (see the
/// <c>&lt;list&gt;</c> case in <see cref="AddElement"/>) so
/// <see cref="UiMarkupList.IconResolve"/> is never set to an
/// always-empty delegate.
/// </summary>
private static Func<uint, (uint tex, int w, int h)>? BuildRowIconResolve(
string? iconKind, IMarkupIconResolver? icons)
private static Func<uint, (uint tex, int w, int h)> BuildRowIconResolve(
string? iconKind, IMarkupIconResolver icons)
{
if (icons is null)
return null;
return (iconKind ?? "did") switch
string kind = ValidateIconKind(iconKind);
return kind switch
{
"did" => id => icons.ResolveDid(
PluginIcons.Normalize(id)),
"spell" => icons.ResolveSpell,
"item" => icons.ResolveItem,
var other => throw new FormatException(
$"unknown iconkind \"{other}\" (expected did, spell, or item)"),
_ => throw new InvalidOperationException(
"unreachable — ValidateIconKind already rejected anything else"),
};
}
/// <summary>
/// Resolves a <c>did</c>/<c>spell</c>/<c>item</c> attribute to a live
/// <see cref="uint"/> reader: a <c>{Prop}</c> binding re-reads a
/// <see cref="uint"/> property every frame; a literal accepts hex
/// (<c>0x...</c>) or decimal, matching every other markup id attribute's
/// "malformed literal throws at Build" rule.
/// <see cref="uint"/> reader: a <c>{Prop}</c> binding re-reads a property
/// every frame; a literal accepts hex (<c>0x...</c>) or decimal, matching
/// every other markup id attribute's "malformed literal throws at Build"
/// rule.
/// </summary>
/// <remarks>
/// Review fix round finding 5: accepts ANY integral property type
/// (<see cref="int"/>, <see cref="long"/>, <see cref="uint"/>,
/// <see cref="ushort"/>, a nullable of any of those, …), not only an
/// exact <see cref="uint"/> match — matching <see cref="BindUint"/>'s own
/// leniency below. Decal-facing bindings are commonly <c>int</c> end to
/// end (e.g. MosswartMassacre's <c>HudPictureBox.Image</c>), so requiring
/// a literal <c>uint</c> property rejected every one of them at Build. A
/// property whose runtime value cannot convert (a non-numeric type) still
/// throws — just from <see cref="Convert.ToUInt32(object, IFormatProvider)"/>
/// at read time rather than a type check at Build, the same tradeoff
/// <see cref="BindUint"/> already makes.
/// </remarks>
private static Func<uint> BindUintLiteralOrBinding(
string expression, object binding, string context)
{
@ -564,13 +637,18 @@ public static class MarkupDocument
return () => literal;
}
PropertyInfo? property = binding.GetType().GetProperty(expression[1..^1]);
if (property is null || property.PropertyType != typeof(uint))
if (property is null)
{
throw new FormatException(
$"{expression} did not resolve to a uint property on "
$"{expression} did not resolve to a property on "
+ binding.GetType().Name + $" ({context})");
}
return () => (uint)property.GetValue(binding)!;
return () => property.GetValue(binding) switch
{
uint u => u,
null => 0u,
var v => Convert.ToUInt32(v, CultureInfo.InvariantCulture),
};
}
private static uint ParseUintLiteral(string text, string context)
@ -692,6 +770,16 @@ public static class MarkupDocument
: Array.Empty<string>();
}
/// <remarks>
/// Review fix round finding 5: also accepts <see cref="IEnumerable{T}"/>
/// of <see cref="int"/> — Decal is <c>int</c> end to end
/// (MosswartMassacre's <c>FlagTrackerView.cs</c> feeds
/// <c>HudPictureBox.Image</c> from <c>int</c> ids), so a plugin porting
/// that convention hands the host <c>IEnumerable&lt;int&gt;</c>, not
/// <c>IEnumerable&lt;uint&gt;</c>. Icon ids never go negative in
/// practice, so the per-element conversion is an unchecked reinterpret
/// rather than a throwing checked cast.
/// </remarks>
private static Func<IReadOnlyList<uint>> BindUintList(
string? expression,
object binding,
@ -702,16 +790,27 @@ public static class MarkupDocument
if (!IsBinding(expression))
throw new FormatException($"{context} must be a uint-list binding");
PropertyInfo? property = binding.GetType().GetProperty(expression[1..^1]);
if (property is null
|| !typeof(IEnumerable<uint>).IsAssignableFrom(property.PropertyType))
if (property is null)
{
throw new FormatException(
$"{expression} did not resolve to an IEnumerable<uint> property on "
+ binding.GetType().Name);
$"{expression} did not resolve to an IEnumerable<uint> or "
+ "IEnumerable<int> property on " + binding.GetType().Name);
}
return () => property.GetValue(binding) is IEnumerable<uint> values
? values.ToArray()
: Array.Empty<uint>();
if (typeof(IEnumerable<uint>).IsAssignableFrom(property.PropertyType))
{
return () => property.GetValue(binding) is IEnumerable<uint> values
? values.ToArray()
: Array.Empty<uint>();
}
if (typeof(IEnumerable<int>).IsAssignableFrom(property.PropertyType))
{
return () => property.GetValue(binding) is IEnumerable<int> values
? values.Select(static v => unchecked((uint)v)).ToArray()
: Array.Empty<uint>();
}
throw new FormatException(
$"{expression} did not resolve to an IEnumerable<uint> or "
+ "IEnumerable<int> property on " + binding.GetType().Name);
}
private static bool IsBinding(string value) =>

37
src/AcDream.App/UI/PluginSidePanel.cs
View file

@ -797,6 +797,20 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState
private readonly Func<uint, (uint tex, int width, int height)> _resolve;
private readonly uint _iconSurfaceId;
private readonly string _tooltip;
private readonly string _initialsFallback;
/// <summary>
/// Review fix round finding 10: whether the FIRST resolve attempt
/// (memoized here, never re-attempted, to avoid re-running
/// <see cref="Initials"/>'s string work every frame) found real art.
/// A non-zero <see cref="_iconSurfaceId"/> whose resolve never
/// succeeds (a bad Decal index, a DAT id from a different
/// installation) falls back to <see cref="_initialsFallback"/> —
/// the same fallback an id of exactly 0 already used — rather than
/// rendering a blank button.
/// </summary>
private bool _iconResolveAttempted;
private bool _iconAvailable;
internal PluginShelfButton(
PluginPanelDescriptor descriptor,
@ -817,9 +831,8 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState
StringComparison.Ordinal)
? descriptor.Title
: $"{ownerDisplayName} — {descriptor.Title}";
Text = _iconSurfaceId == 0
? Initials(descriptor.IconText, descriptor.Title)
: string.Empty;
_initialsFallback = Initials(descriptor.IconText, descriptor.Title);
Text = _iconSurfaceId == 0 ? _initialsFallback : string.Empty;
DatFont = font;
Outline = true;
BorderThickness = 1f;
@ -838,8 +851,24 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState
protected override void OnDraw(UiRenderContext ctx)
{
// Decided once, on the first draw: a non-zero id whose resolve
// never yields a texture falls back to initials permanently
// (rather than re-attempting — and re-allocating Initials'
// string — every frame). Text must be settled BEFORE
// base.OnDraw runs, since that call is what actually draws the
// caption.
if (!_iconResolveAttempted && _iconSurfaceId != 0)
{
_iconResolveAttempted = true;
(uint tex, int w, int h) = _resolve(_iconSurfaceId);
_iconAvailable = tex != 0 && w > 0 && h > 0;
if (!_iconAvailable)
Text = _initialsFallback;
}
base.OnDraw(ctx);
if (_iconSurfaceId == 0)
if (_iconSurfaceId == 0 || !_iconAvailable)
return;
(uint texture, int width, int height) = _resolve(_iconSurfaceId);

10
src/AcDream.App/UI/RetailUiRuntime.cs
View file

@ -39,7 +39,12 @@ public sealed record RetailUiAssets(
UiDatFont? DefaultFont,
BitmapFont? DebugFont,
ControlsIni Controls,
IconComposer Icons);
IconComposer Icons,
// Review fix round (Slice B finding 1 + 6): RetailMarkupIconResolver
// needs the nearest-sampled upload path (nearest:true) that ResolveSprite
// above never exposes (it is ResolveChrome, always nearest:false for
// background/border art) — see that resolver's own ResolveDid doc.
TextureCache TextureCache);
public sealed record VitalsRuntimeBindings(VitalsVM ViewModel);
@ -4720,7 +4725,8 @@ public sealed class RetailUiRuntime : IDisposable
// RetailMarkupIconResolver's own doc comment for exactly which
// existing bindings field supplies the object table.
IMarkupIconResolver iconResolver = new RetailMarkupIconResolver(
_bindings.Assets.ResolveSprite,
_bindings.Assets.Dats,
_bindings.Assets.TextureCache,
_bindings.Assets.Icons,
_bindings.Toolbar.Objects);

18
src/AcDream.App/UI/UiPanel.cs
View file

@ -222,17 +222,24 @@ public class UiSimpleButton : UiPanel
{
base.OnDraw(ctx);
// Slice B icon column: reserved only when an icon actually resolved
// this frame, so a bound id that goes to 0 (or the resolver returning
// no texture) collapses back to the un-iconed centering — never a
// permanent blank gap.
// Slice B icon column: review fix round finding 7 — the column is
// reserved whenever IconSource is SET (non-null), regardless of
// whether the icon actually resolved THIS frame. MarkupDocument only
// ever assigns IconSource when a real IMarkupIconResolver is wired
// (see its <button icon> call site), so a set-but-momentarily-
// unresolved id (a bound value that just went to 0, a DAT lookup
// still warming up) no longer slides the caption back and forth
// frame to frame — only the sprite draw itself is conditional on a
// successful resolve.
float iconColumn = 0f;
if (IconSource is { } iconSource)
{
float extent = MathF.Max(0f, MathF.Min(Width, Height) - 6f);
iconColumn = extent + 6f;
(uint tex, int w, int h) = iconSource();
if (tex != 0u && w > 0 && h > 0)
{
float extent = MathF.Max(0f, MathF.Min(Width, Height) - 6f);
float scale = MathF.Min(extent / w, extent / h);
float drawWidth = w * scale;
float drawHeight = h * scale;
@ -242,7 +249,6 @@ public class UiSimpleButton : UiPanel
(Height - drawHeight) * 0.5f,
drawWidth, drawHeight,
0f, 0f, 1f, 1f, Vector4.One);
iconColumn = extent + 6f;
}
}

7
src/AcDream.Plugin.Abstractions/IUiRegistry.cs
View file

@ -24,6 +24,13 @@ public sealed record PluginPanelDescriptor(string WindowId, string Title)
/// <see cref="IconText"/> instead. Plugins never receive the resulting GPU
/// resource and remain BCL-only.
/// </summary>
/// <remarks>
/// Accepts a bare Decal/VirindiViewService-style portal.dat index as well
/// as a full RenderSurface DID — the host normalizes every value through
/// <see cref="PluginIcons.Normalize"/> before drawing it, the same
/// grammar applied at plugin markup's <c>&lt;icon did&gt;</c>/
/// <c>&lt;button icon&gt;</c>/<c>&lt;list icons&gt;</c> sinks.
/// </remarks>
public uint IconSurfaceId { get; init; }
/// <summary>

39
src/AcDream.Plugin.Abstractions/PluginIcons.cs
View file

@ -6,10 +6,10 @@ namespace AcDream.Plugin.Abstractions;
/// markup's <c>&lt;icon did&gt;</c>, <c>&lt;button icon&gt;</c>, and
/// <c>&lt;list icons&gt;</c>). Decal/VirindiViewService plugins (the
/// reference usage: MosswartMassacre's <c>HudPictureBox.Image</c> assignments)
/// hand out bare portal.dat indices — small integers below the
/// <c>0x06xxxxxx</c> RenderSurface DID range, the same numbers Decal's
/// <c>FileService.SpellTable</c>/<c>SkillTable</c> icon columns return. A
/// host that draws those literally as RenderSurface DIDs resolves nothing.
/// hand out bare portal.dat indices — small integers below <c>0x01000000</c>,
/// the same numbers Decal's <c>FileService.SpellTable</c>/<c>SkillTable</c>
/// icon columns return. A host that draws those literally as RenderSurface
/// DIDs (the <c>0x06xxxxxx</c> block) resolves nothing.
/// </summary>
/// <remarks>
/// This grammar is deliberately host-side and applied at the SINK, not at
@ -25,25 +25,36 @@ namespace AcDream.Plugin.Abstractions;
public static class PluginIcons
{
/// <summary>
/// The first RenderSurface DID range (portal.dat's <c>0x06000000</c>
/// block). Any value below this is treated as a bare index needing the
/// block prefix; any value at or above it is assumed to already be a
/// resolvable DID (RenderSurface or otherwise) and is returned unchanged.
/// The bare-index/DID boundary: <c>0x01000000</c>. Decal/VVS icon indices
/// (portal.dat's SpellTable/SkillTable icon columns, and the raw
/// integers a plugin author types by hand) are always well below this —
/// portal.dat's own id space reserves everything from <c>0x01000000</c>
/// up for named DBTYPE blocks (RenderSurface's own block starts at
/// <c>0x06000000</c>, comfortably above it). A value below the boundary
/// is treated as a bare index needing the RenderSurface block prefix
/// added; a value at or above it is assumed to already be a resolvable
/// DID (RenderSurface or otherwise) and is returned unchanged.
/// </summary>
private const uint BareIndexBoundary = 0x01000000u;
/// <summary>
/// The RenderSurface DID block prefix added to a bare index below
/// <see cref="BareIndexBoundary"/>.
/// </summary>
private const uint RenderSurfaceBlock = 0x06000000u;
/// <summary>
/// Normalizes one plugin-supplied icon id: <c>0</c> stays <c>0</c> (no
/// icon); a bare index below <see cref="RenderSurfaceBlock"/> becomes
/// <c>0x06000000 + value</c> (Decal/VVS convention); anything else is
/// returned unchanged (already a full DID, of whatever DBTYPE the caller
/// intends — the host resolves it against the space its own sink
/// expects).
/// icon); a bare index below <c>0x01000000</c> becomes
/// <c>0x06000000 + value</c> (Decal/VVS convention); a value at or above
/// <c>0x01000000</c> — including <c>0x01000000</c> itself — is returned
/// unchanged (already a full DID, of whatever DBTYPE the caller intends —
/// the host resolves it against the space its own sink expects).
/// </summary>
public static uint Normalize(uint idOrIndex) =>
idOrIndex == 0u
? 0u
: idOrIndex < RenderSurfaceBlock
: idOrIndex < BareIndexBoundary
? RenderSurfaceBlock + idOrIndex
: idOrIndex;
}

100
src/AcDream.Plugins.Smoke/SmokeIconPanel.cs
View file

@ -9,21 +9,36 @@ namespace AcDream.Plugins.Smoke;
/// (no plugin-side XML file), with the descriptor's own
/// <see cref="PluginPanelDescriptor.IconSurfaceId"/> also set to the bare
/// Decal-style index used below, so the shelf button and the panel's own
/// <c>&lt;icon did="7735"&gt;</c> prove
/// <c>&lt;icon did="0x165"&gt;</c> prove
/// <see cref="PluginIcons.Normalize"/> the same way at both sinks.
/// </summary>
/// <remarks>
/// Review fix round (2026-09-06): <see cref="BareIndexIconId"/> was
/// <c>7735</c> (an unverified literal) and is now retail's Melee Defense
/// skill icon index <c>0x165</c> — <c>SampleData.cs:64</c> attests
/// <c>0x06000165</c> is a real installed-DAT RenderSurface, so the
/// descriptor and the bare-index <c>&lt;icon&gt;</c> both draw ART a tester
/// can actually verify against retail, not a guess. The list's
/// <c>iconkind="spell"</c> column also had a wiring bug: it fed
/// <see cref="PluginSpellInfo.IconId"/> (a raw RenderSurface DID) to the
/// SPELL-id resolver, which composites a badge from a SpellTable entry
/// looked up by SPELL id — the two numbers are unrelated, so every row
/// silently resolved the wrong (or no) composited badge. See
/// <see cref="Binding.SpellIds"/>.
/// </remarks>
internal static class SmokeIconPanel
{
/// <summary>
/// A Decal/VirindiViewService-style bare portal.dat index (MosswartMassacre's
/// convention — see the plan's "Why" section) rather than a full
/// <c>0x06xxxxxx</c> RenderSurface DID. Deliberately used on BOTH the
/// descriptor and the first <c>&lt;icon&gt;</c> below to prove the host
/// normalizes it identically at each sink.
/// A Decal/VirindiViewService-style bare portal.dat index for retail's
/// Melee Defense skill icon (<c>0x06000165</c> — attested in
/// <c>src/AcDream.App/UI/Layout/SampleData.cs:64</c>) rather than an
/// unverified literal. Deliberately used on BOTH the descriptor and the
/// first <c>&lt;icon&gt;</c> below to prove the host normalizes it
/// identically at each sink.
/// </summary>
public const uint BareIndexIconId = 7735u;
public const uint BareIndexIconId = 0x165u;
/// <summary>A literal, already-normalized RenderSurface DID.</summary>
/// <summary>A literal, already-normalized RenderSurface DID (a portal icon).</summary>
private const uint LiteralDidIconId = 0x06002D14u;
/// <summary>
@ -35,17 +50,17 @@ internal static class SmokeIconPanel
public static readonly PluginPanelDescriptor Descriptor = new("icons", "Icon Smoke")
{
IconSurfaceId = BareIndexIconId,
StartVisible = false,
StartVisible = true,
ShowInSidePanel = true,
};
public const string Markup = """
<panel x="60" y="60" w="280" h="220" title="Icon Smoke">
<icon x="12" y="28" w="32" h="32" did="7735" tooltip="bare Decal index"/>
<icon x="52" y="28" w="32" h="32" did="0x06002D14" tooltip="literal RenderSurface DID"/>
<icon x="92" y="28" w="32" h="32" spell="{SpellId}" tooltip="composited spell icon"/>
<button x="12" y="68" w="120" h="24" text="Report" icon="0x06002D14" onclick="{Report}"/>
<list x="12" y="100" w="256" h="108" items="{SpellRows}" icons="{SpellIconIds}"
<icon x="12" y="28" w="32" h="32" did="0x165" tooltip="Melee Defense skill icon (bare index 0x165)"/>
<icon x="52" y="28" w="32" h="32" did="0x06002D14" tooltip="Portal icon 0x06002D14 (literal RenderSurface DID)"/>
<icon x="92" y="28" w="32" h="32" spell="{SpellId}" tooltip="composited spell icon (first known self-buff, or Strength Other I)"/>
<button x="12" y="68" w="120" h="24" text="Report" icon="0x06002D14" iconkind="did" onclick="{Report}"/>
<list x="12" y="100" w="256" h="108" items="{SpellRows}" icons="{SpellIds}"
iconkind="spell" selected="{SelectedIndex}"/>
</panel>
""";
@ -73,16 +88,47 @@ internal static class SmokeIconPanel
}
}
/// <summary>Parallel icon-id column for <see cref="SpellRows"/>: the
/// first five known self-buffs' raw SpellTable icon DIDs.</summary>
public IEnumerable<uint> SpellIconIds
/// <summary>
/// The first five known self-buffs, or — when the character has
/// learned nothing yet — a single-entry fallback to
/// <see cref="FallbackSpellId"/>'s REAL catalog entry (never
/// fabricated art: if the host cannot resolve spell 1 either, the
/// fallback is empty and <see cref="SpellRows"/>/<see cref="SpellIds"/>
/// show the honest "no spells known" row instead).
/// </summary>
private IReadOnlyList<PluginSpellInfo> KnownSelfBuffsOrFallback()
{
IReadOnlyList<PluginSpellInfo> known = _host.Automation.Spells.KnownSelfBuffs;
if (known.Count > 0)
return known;
return _host.Automation.Spells.TryGet(FallbackSpellId, out PluginSpellInfo info)
? new[] { info }
: Array.Empty<PluginSpellInfo>();
}
/// <summary>
/// Parallel spell-id column for <see cref="SpellRows"/>, feeding the
/// list's <c>iconkind="spell"</c> composited badge. Review fix round
/// finding 2: this must be the spell id
/// (<see cref="PluginSpellInfo.SpellId"/>) that
/// <see cref="IMarkupIconResolver.ResolveSpell"/> composites a badge
/// from — NOT <see cref="PluginSpellInfo.IconId"/> (the spell's raw
/// art DID), which is what this property used to yield. The raw
/// <see cref="PluginSpellInfo.IconId"/> is still visible, printed
/// alongside the name in <see cref="SpellRows"/>, for anyone
/// comparing the composited badge against the plain art tile.
/// </summary>
public IEnumerable<uint> SpellIds
{
get
{
IReadOnlyList<PluginSpellInfo> spells = KnownSelfBuffsOrFallback();
if (spells.Count == 0)
return new uint[] { 0u };
var ids = new List<uint>();
foreach (PluginSpellInfo spell in _host.Automation.Spells.KnownSelfBuffs)
foreach (PluginSpellInfo spell in spells)
{
ids.Add(spell.IconId);
ids.Add(spell.SpellId);
if (ids.Count == 5)
break;
}
@ -90,16 +136,22 @@ internal static class SmokeIconPanel
}
}
/// <summary>Row text for the first five known self-buffs, with each
/// spell's <see cref="PluginSpellInfo.IconId"/> printed alongside its
/// name so the icon column and the raw id are both visible in one
/// look.</summary>
/// <summary>Row text for the first five known self-buffs (or the
/// single-entry fallback), with each spell's
/// <see cref="PluginSpellInfo.IconId"/> printed alongside its name so
/// the composited icon column and the raw art id are both visible in
/// one look. A character with no self-buffs known AND no resolvable
/// fallback shows one honest "no spells known" row rather than a
/// blank list.</summary>
public IEnumerable<string> SpellRows
{
get
{
IReadOnlyList<PluginSpellInfo> spells = KnownSelfBuffsOrFallback();
if (spells.Count == 0)
return new[] { "no spells known" };
var rows = new List<string>();
foreach (PluginSpellInfo spell in _host.Automation.Spells.KnownSelfBuffs)
foreach (PluginSpellInfo spell in spells)
{
rows.Add($"{spell.Name} (icon 0x{spell.IconId:X8})");
if (rows.Count == 5)

43
tests/AcDream.App.Tests/Plugins/AppAutomationSurfaceTests.cs
View file

@ -274,4 +274,47 @@ public sealed class AppAutomationSurfaceTests
public void DisplayMessage(string message) { }
public void IncrementBusy() { }
}
/// <summary>
/// Review fix round finding 14 (docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md
/// Slice B): <see cref="PluginWorldObject.IconId"/> must carry
/// <see cref="ClientObject.IconId"/> through <c>AppAutomationSurface.ProjectWorldObject</c>
/// unchanged. That projector is private and its public callers
/// (<see cref="IWorldObjectAutomation.CaptureObjects"/>/<c>TryGet</c>)
/// gate on <see cref="AppAutomationSurface.IsAvailable"/>, which requires
/// a fully connected session (<c>RuntimeLifecycleState.InWorld</c>) —
/// heavier than this mapping needs. Per the plan's own fallback ("an
/// InstalledDat-free unit on the projection function is enough"), this
/// invokes the private projector directly via reflection: an
/// InstalledDat-free unit on exactly the projection function.
/// <see cref="PluginInventoryItem.IconId"/> is filled by the identical
/// one-line pattern (<c>item.IconId</c>) inline in
/// <c>CaptureOwnedItems</c>, reviewed by inspection rather than
/// duplicated here.
/// </summary>
[Fact]
public void ProjectWorldObject_FillsIconIdFromTheClientObject()
{
using var runtime = GameRuntimeTestFactory.Create();
using var surface = new AppAutomationSurface();
var item = new ClientObject
{
ObjectId = 0x50000456u,
Name = "Test Item",
IconId = 0x06000165u,
};
System.Reflection.MethodInfo method = typeof(AppAutomationSurface)
.GetMethod(
"ProjectWorldObject",
System.Reflection.BindingFlags.NonPublic
| System.Reflection.BindingFlags.Instance)
?? throw new InvalidOperationException(
"AppAutomationSurface.ProjectWorldObject was not found by reflection.");
var result = (PluginWorldObject)method.Invoke(
surface,
[runtime, null, item, 0u])!;
Assert.Equal(0x06000165u, result.IconId);
}
}

4
tests/AcDream.App.Tests/Plugins/PluginIconsTests.cs
View file

@ -15,7 +15,9 @@ public sealed class PluginIconsTests
[Theory]
[InlineData(0u, 0u)]
[InlineData(7735u, 0x06001E37u)] // Decal-style bare index -> RenderSurface DID
[InlineData(0x00FFFFFFu, 0x06FFFFFFu)] // largest bare index -> still gets the block prefix
[InlineData(0x00FFFFFFu, 0x06FFFFFFu)] // largest bare index (just below the boundary) -> still gets the block prefix
[InlineData(0x01000000u, 0x01000000u)] // exactly at the boundary -> passes through unchanged
[InlineData(0x02000000u, 0x02000000u)] // above the boundary but below the RenderSurface block -> unchanged (distinguishes the restored 0x01000000 threshold from the old 0x06000000 one)
[InlineData(0x06002D14u, 0x06002D14u)] // already a RenderSurface DID -> unchanged
[InlineData(0x0600FFFFu, 0x0600FFFFu)] // already at/above the block -> unchanged
public void Normalize_MapsAccordingToTheGrammar(uint input, uint expected)

190
tests/AcDream.App.Tests/Rendering/TextureCacheRenderSurfaceSamplerKeyTests.cs Normal file
View file

@ -0,0 +1,190 @@
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()
{
}
}
}

223
tests/AcDream.App.Tests/UI/MarkupIconTests.cs
View file

@ -3,6 +3,7 @@ using AcDream.App.Rendering;
using AcDream.App.Rendering.Gpu;
using AcDream.App.Tests.Rendering.Gpu;
using AcDream.App.UI;
using AcDream.Plugin.Abstractions;
using DatReaderWriter.Types;
using Xunit;
@ -273,6 +274,67 @@ public sealed class MarkupIconTests
Assert.Null(button.IconSource);
}
[Fact]
public void ButtonIcon_UnknownIconKind_ThrowsAtBuild_EvenWithNoResolverWired()
{
// Review fix round finding 4: a malformed iconkind is an author
// error regardless of whether the host happens to have icon support
// turned on.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<button x=\"0\" y=\"0\" w=\"60\" h=\"20\" text=\"Go\" " +
"icon=\"1\" iconkind=\"spel\"/>" +
"</panel>";
Assert.Throws<FormatException>(
() => MarkupDocument.Build(xml, new object(), Sprite));
}
[Fact]
public void ButtonIcon_NoResolverWired_IconSourceStaysNull()
{
// Review fix round finding 7: IconSource must be set ONLY when a
// resolver actually exists — otherwise UiSimpleButton (which now
// reserves its icon column whenever IconSource is non-null) would
// permanently reserve a column that never draws anything on a host
// with no icon support wired.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<button x=\"0\" y=\"0\" w=\"60\" h=\"20\" text=\"Go\" icon=\"1\"/>" +
"</panel>";
var panel = MarkupDocument.Build(xml, new object(), Sprite);
var button = Assert.IsType<UiSimpleButton>(panel.Children[0]);
Assert.Null(button.IconSource);
}
private sealed class IntIconBinding
{
public int IconIdInt { get; set; } = 42;
}
[Fact]
public void ButtonIcon_BindsToAnIntProperty_NotOnlyUint()
{
// Review fix round finding 5: Decal-facing bindings are commonly
// `int` end to end (MosswartMassacre's HudPictureBox.Image), so
// requiring an exact `uint` property rejected every one of them at
// Build.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<button x=\"0\" y=\"0\" w=\"60\" h=\"20\" text=\"Go\" icon=\"{IconIdInt}\"/>" +
"</panel>";
var resolver = new FakeIconResolver();
var binding = new IntIconBinding();
var panel = MarkupDocument.Build(xml, binding, Sprite, icons: resolver);
var button = Assert.IsType<UiSimpleButton>(panel.Children[0]);
button.IconSource!();
Assert.Equal(("did", PluginIcons.Normalize(42u)), resolver.Calls[^1]);
}
// ── <list icons>: resolver dispatch + column reservation ──────────────────
private sealed class ListIconBinding
@ -324,6 +386,113 @@ public sealed class MarkupIconTests
Assert.Null(list.IconIdsSource);
}
[Fact]
public void ListIcons_UnknownIconKind_ThrowsAtBuild_EvenWithNoResolverWired()
{
const string xml =
"<panel x=\"0\" y=\"0\" w=\"200\" h=\"100\">" +
"<list x=\"0\" y=\"0\" w=\"180\" h=\"60\" items=\"{Items}\" " +
"icons=\"{IconIds}\" iconkind=\"spel\" selected=\"{Selected}\"/>" +
"</panel>";
Assert.Throws<FormatException>(
() => MarkupDocument.Build(xml, new ListIconBinding(), Sprite));
}
[Fact]
public void ListIcons_NoResolverWired_IconIdsSourceStaysNull()
{
// Review fix round finding 7: same rule as the button — IconIdsSource
// (and IconResolve) must be set ONLY when a resolver actually
// exists, since UiMarkupList already reserves its icon column
// whenever IconIdsSource is non-null.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"200\" h=\"100\">" +
"<list x=\"0\" y=\"0\" w=\"180\" h=\"60\" items=\"{Items}\" " +
"icons=\"{IconIds}\" selected=\"{Selected}\"/>" +
"</panel>";
var panel = MarkupDocument.Build(xml, new ListIconBinding(), Sprite);
var list = Assert.IsType<UiMarkupList>(panel.Children[0]);
Assert.Null(list.IconIdsSource);
Assert.Null(list.IconResolve);
}
private sealed class IntListIconBinding
{
public IReadOnlyList<string> Items { get; } = new[] { "First", "Second" };
public IEnumerable<int> IconIds { get; } = new[] { 7735, 0 };
public int Selected { get; set; } = -1;
}
[Fact]
public void ListIcons_BindsToAnIEnumerableOfInt_NotOnlyIEnumerableOfUint()
{
// Review fix round finding 5: Decal is int end to end.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"200\" h=\"100\">" +
"<list x=\"0\" y=\"0\" w=\"180\" h=\"60\" items=\"{Items}\" " +
"icons=\"{IconIds}\" iconkind=\"did\" selected=\"{Selected}\"/>" +
"</panel>";
var resolver = new FakeIconResolver();
var binding = new IntListIconBinding();
var panel = MarkupDocument.Build(xml, binding, Sprite, icons: resolver);
var list = Assert.IsType<UiMarkupList>(panel.Children[0]);
Assert.NotNull(list.IconIdsSource);
Assert.Equal(new uint[] { 7735u, 0u }, list.IconIdsSource!());
}
// ── <icon tooltip>: empty tooltip must not swallow clicks ─────────────────
[Fact]
public void Icon_EmptyTooltip_StaysClickThrough()
{
// Review fix round finding 9: an empty tooltip="" must not make the
// icon a hit-test target — match ApplyCommon's own
// !IsNullOrWhiteSpace predicate rather than a bare attribute-
// presence check.
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<icon x=\"0\" y=\"0\" did=\"1\" tooltip=\"\"/>" +
"</panel>";
var panel = MarkupDocument.Build(xml, new object(), Sprite);
var icon = Assert.IsType<UiMarkupIcon>(panel.Children[0]);
Assert.True(icon.ClickThrough);
}
[Fact]
public void Icon_NonEmptyTooltip_BecomesARealHitTestTarget()
{
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<icon x=\"0\" y=\"0\" did=\"1\" tooltip=\"real tooltip\"/>" +
"</panel>";
var panel = MarkupDocument.Build(xml, new object(), Sprite);
var icon = Assert.IsType<UiMarkupIcon>(panel.Children[0]);
Assert.False(icon.ClickThrough);
}
// ── Unknown element names throw at Build (finding 11) ─────────────────────
[Fact]
public void UnknownElementName_ThrowsFormatExceptionAtBuild()
{
const string xml =
"<panel x=\"0\" y=\"0\" w=\"100\" h=\"60\">" +
"<butotn x=\"0\" y=\"0\" w=\"60\" h=\"20\" text=\"Go\"/>" +
"</panel>";
Assert.Throws<FormatException>(
() => MarkupDocument.Build(xml, new object(), Sprite));
}
// ── Draw-level: "draws nothing" / "shifts text" pinned against real quads ─
private sealed class NullGpuFrameSource : ICurrentGpuFrameSource
@ -449,4 +618,58 @@ public sealed class MarkupIconTests
// it, never overlapping where row text starts.
Assert.True(iconQuad.Verts[8] <= list.RowHeight - 2f + 0.01f);
}
[Fact]
public void UiMarkupList_WithIconColumn_TextStartsRightOfWithoutColumn_AndIconHasNonZeroWidth()
{
// Review fix round finding 8: a "with vs. without icons" comparison
// pin — the button-icon test above already proves this shape for
// UiSimpleButton; the list's own icon column (a SEPARATE code path
// in UiMarkupList.OnDraw) had no equivalent pin until now, so a
// regression that stopped shifting the text column (or drew a
// zero-width icon) would have passed every existing list test.
var glyphs = new Dictionary<char, FontCharDesc>
{
['G'] = new FontCharDesc { Unicode = 'G', Width = 8, Height = 8 },
};
var font = new UiDatFont(
fgTex: 1u, fgW: 32, fgH: 32,
bgTex: 0, bgW: 0, bgH: 0,
lineHeight: 16f, baselineOffset: 12f,
glyphs);
var withoutIcons = new UiMarkupList
{
Width = 180f, Height = 60f, RowHeight = 18f, DatFont = font,
ItemsSource = () => new[] { "G" },
BackgroundColor = default, BorderColor = default,
};
var withIcons = new UiMarkupList
{
Width = 180f, Height = 60f, RowHeight = 18f, DatFont = font,
ItemsSource = () => new[] { "G" },
IconIdsSource = () => new uint[] { 9u },
IconResolve = id => (id, 16, 16),
BackgroundColor = default, BorderColor = default,
};
var (rendererWithout, ctxWithout) = MakeContext(200f, 200f);
withoutIcons.DrawSelfAndChildren(ctxWithout);
var (rendererWith, ctxWith) = MakeContext(200f, 200f);
withIcons.DrawSelfAndChildren(ctxWith);
// The icon sprite itself drew, with a real (non-zero) width.
var iconQuad = Assert.Single(rendererWith.DebugSpriteSegmentVerts, s => s.Texture == 9u);
float iconWidth = iconQuad.Verts[8] - iconQuad.Verts[0];
Assert.True(iconWidth > 0f, $"expected a non-zero icon quad width, got {iconWidth}");
// Both lists draw exactly one glyph quad (font texture 1u) for row 0's
// text; the icon-bearing list's text starts strictly to the right of
// the icon-less list's, because the reserved icon column shifted it.
var textWithout = Assert.Single(rendererWithout.DebugSpriteSegmentVerts, s => s.Texture == 1u);
var textWith = Assert.Single(rendererWith.DebugSpriteSegmentVerts, s => s.Texture == 1u);
Assert.True(
textWith.Verts[0] > textWithout.Verts[0],
$"expected the icon-bearing list's text ({textWith.Verts[0]}) to start right of the icon-less list's ({textWithout.Verts[0]})");
}
}

90
tests/AcDream.App.Tests/UI/PluginSidePanelTests.cs
View file

@ -1,4 +1,7 @@
using System.Numerics;
using AcDream.App.Rendering;
using AcDream.App.Rendering.Gpu;
using AcDream.App.Tests.Rendering.Gpu;
using AcDream.App.UI;
using AcDream.Plugin.Abstractions;
using AcDream.UI.Abstractions.Panels.Settings;
@ -971,4 +974,91 @@ public sealed class PluginSidePanelTests
// User-positioned now: the LEFT edge survives instead.
Assert.Equal(leftAfterDrag, shelf.Left);
}
[Fact]
public void ShelfButton_NormalizesADecalBareIndexDescriptorId_BeforeResolving()
{
// Review fix round finding 14 (docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md
// Slice B): the descriptor's bare Decal-style index must resolve
// through PluginIcons.Normalize at the shelf button, same as every
// markup did sink — 0x165 (Melee Defense's real installed-DAT index,
// SampleData.cs:64) becomes 0x06000165.
var resolvedIds = new List<uint>();
var root = new UiRoot { Width = 800f, Height = 600f };
using var shelf = new PluginSidePanel(
root.WindowManager,
id => { resolvedIds.Add(id); return (9u, 32, 32); },
font: null);
root.AddChild(shelf);
var frame = new UiPanel { Width = 200f, Height = 100f };
root.AddChild(frame);
RetailWindowHandle handle = root.WindowManager.Register(
"plugin:acdream.test:main", frame);
shelf.Add(
new PluginUiOwner("acdream.test", "Test Plugin"),
new PluginPanelDescriptor("main", "Test Plugin")
{
IconSurfaceId = 0x165u,
},
handle);
PluginSidePanel.PluginShelfButton button = Assert.Single(
shelf.Children.OfType<PluginSidePanel.PluginShelfButton>());
button.DrawSelfAndChildren(TestUiRenderContext());
Assert.Contains(0x06000165u, resolvedIds);
}
[Fact]
public void ShelfButton_FallsBackToInitials_WhenTheResolveNeverYieldsATexture()
{
// Review fix round finding 10: a non-zero descriptor icon id whose
// resolve never succeeds (a bad Decal index, a DAT id from a
// different installation) must fall back to the initials text
// rather than rendering a blank button forever.
var root = new UiRoot { Width = 800f, Height = 600f };
using var shelf = new PluginSidePanel(
root.WindowManager,
_ => (0u, 0, 0),
font: null);
root.AddChild(shelf);
var frame = new UiPanel { Width = 200f, Height = 100f };
root.AddChild(frame);
RetailWindowHandle handle = root.WindowManager.Register(
"plugin:acdream.test:main", frame);
shelf.Add(
new PluginUiOwner("acdream.test", "Test Plugin"),
new PluginPanelDescriptor("main", "Test Plugin")
{
IconText = "TP",
IconSurfaceId = 0x165u,
},
handle);
PluginSidePanel.PluginShelfButton button = Assert.Single(
shelf.Children.OfType<PluginSidePanel.PluginShelfButton>());
// Before any draw, the button still shows empty text (a non-zero
// icon id was supplied and has not been attempted yet).
Assert.Equal(string.Empty, button.Text);
button.DrawSelfAndChildren(TestUiRenderContext());
Assert.Equal("TP", button.Text);
}
private static UiRenderContext TestUiRenderContext()
{
var device = new RecordingGpuDevice();
var renderer = new TextRenderer(
device, new NullGpuFrameSource(), "unused");
renderer.Begin(new Vector2(200f, 200f));
return new UiRenderContext(renderer, new Vector2(200f, 200f));
}
private sealed class NullGpuFrameSource : ICurrentGpuFrameSource
{
public IGpuFrame? CurrentFrame => null;
}
}

65
tests/AcDream.App.Tests/UI/RetailMarkupIconResolverInstalledDatTests.cs Normal file
View file

@ -0,0 +1,65 @@
using AcDream.App.Rendering;
using AcDream.App.Tests.Rendering;
using AcDream.App.Tests.Rendering.Gpu;
using AcDream.App.UI;
using AcDream.Content;
using AcDream.Core.Items;
using DatReaderWriter.Options;
using Xunit;
namespace AcDream.App.Tests.UI;
/// <summary>
/// Review fix round finding 1 (<c>docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md</c>
/// Slice B): <see cref="RetailMarkupIconResolver.ResolveDid"/> must return
/// <c>(0, 0, 0)</c> for an id that does not resolve to a real installed
/// RenderSurface — NEVER fall through to
/// <see cref="TextureCache.GetOrUploadRenderSurface"/>'s 1x1 magenta
/// placeholder, which is load-bearing for authored chrome and would
/// otherwise get scaled up to a full magenta square by
/// <see cref="UiMarkupIcon"/>/<see cref="UiMarkupList"/>/<see cref="UiSimpleButton"/>
/// (see <c>claude-memory/feedback_ui_resolve_zero_magenta.md</c>: guard on
/// the id, never on the resolved handle).
/// </summary>
[Trait("Lane", "InstalledDat")]
public sealed class RetailMarkupIconResolverInstalledDatTests
{
/// <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:64</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>Retail's Melee Defense skill icon — a known-real installed DID.</summary>
private const uint KnownRealDid = 0x06000165u;
[Fact]
public void ResolveDid_UnresolvableId_ReturnsNothing_AndKnownRealId_ReturnsATexture()
{
string? datDir = InstalledDatTestPath.Resolve();
if (datDir is null)
{
Assert.Fail(
"Lane=InstalledDat requires an installed retail DAT directory; see docs/release-gate.md.");
return;
}
using var dats = new DatCollection(datDir, DatAccessType.Read);
using var adapter = new DatCollectionAdapter(dats);
var device = new RecordingGpuDevice();
using var cache = new TextureCache(device, adapter);
var icons = new IconComposer(adapter, cache);
var objects = new ClientObjectTable();
var resolver = new RetailMarkupIconResolver(adapter, cache, icons, objects);
(uint missTex, int missW, int missH) = resolver.ResolveDid(DecalHabitDoubleNormalizedId);
Assert.Equal((0u, 0, 0), (missTex, missW, missH));
(uint realTex, int realW, int realH) = resolver.ResolveDid(KnownRealDid);
Assert.NotEqual(0u, realTex);
Assert.True(realW > 0 && realH > 0);
}
}