fix #420: seed face-segment media states so character select stops crashing the client
Every launcher-started play session on 2026-08-19 died a few seconds after
login. The user's own session evidence shows it three times in a row:
started -> connected -> characterList -> exited code 1 "crashed", with
client.err.log carrying
System.ArgumentNullException: Value cannot be null. (Parameter 'key')
at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
at AcDream.App.UI.UiButton.OnDraw(UiRenderContext ctx)
UiButton allocated its per-face-segment media-state array as `new string[n]`,
leaving every element null, while the single-face sibling _faceMediaState was
correctly seeded to "" (DirectState). NextMediaState returns `current`
unchanged on three of its four arms — including retail's own "committed state
authored with an empty media array keeps the previous media playing" rule — so
on a multi-segment button whose committed state carries no media the null
survived the first SyncMediaStates and reached
ElementInfo.StateMedia.TryGetValue(null), throwing mid-paint and taking the
process down.
Seed the array with "" at construction. That is what the constructor's
existing comment already claimed the media machine did ("the media machine
begins on the element's BASE media"); only the segment array was left out.
Verified by reverting the one-line fix: the new regression test throws
ArgumentNullException from UiButton.ActiveFile, the same frame as the live
crash. AcDream.App.Tests UiButton filter: 41 passed, 3 skipped.
Found while investigating Campaign LU item 4 ("launching the selected
character doesn't work") — this is why nothing worked. Also lands the Campaign
LU plan doc, whose recon section records the mechanisms the remaining slices
build on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
a3b0455f59
commit
a34e8f2a17
4 changed files with 344 additions and 0 deletions
|
|
@ -24,6 +24,46 @@ What does NOT go here:
|
|||
- Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending.
|
||||
- Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed.
|
||||
|
||||
## #420 — Client crashes on the character-select screen (`UiButton.OnDraw` null media-state key)
|
||||
|
||||
**Status:** ✅ FIXED 2026-08-19, root cause proven by a reverting test.
|
||||
**Symptom:** every launcher-started play session died seconds after login.
|
||||
The user's own session evidence (`%LOCALAPPDATA%\acdream\cache\launcher\
|
||||
sessions\*/`) shows the exact shape three times in a row on 2026-08-19:
|
||||
`started` → `connected` → `characterList` (2 characters) →
|
||||
`exited code 1 "crashed"`, with `client.err.log` carrying
|
||||
|
||||
```
|
||||
Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'key')
|
||||
at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
|
||||
at AcDream.App.UI.UiButton.OnDraw(UiRenderContext ctx)
|
||||
```
|
||||
|
||||
**Root cause:** `UiButton`'s constructor allocated the per-face-segment
|
||||
media-state array as `new string[n]`, leaving every element **null**, while
|
||||
its single-face sibling `_faceMediaState` was correctly seeded to `""`
|
||||
(DirectState). `NextMediaState` returns `current` UNCHANGED on three of its
|
||||
four arms — including "the committed state is authored but its media array is
|
||||
empty", which is retail's own keep-playing-the-previous-media rule. So on a
|
||||
multi-segment button whose committed state carries no media, the null
|
||||
survived the first `SyncMediaStates` and reached
|
||||
`ElementInfo.StateMedia.TryGetValue(null)`, throwing mid-paint and taking the
|
||||
process down.
|
||||
|
||||
**Fix:** `Array.Fill(_segmentMediaStates, "")` at construction — the segment
|
||||
array now starts on base media exactly like `_faceMediaState`, which is what
|
||||
the surrounding comment already claimed the media machine did.
|
||||
|
||||
**Regression test:**
|
||||
`UiButtonTests.MultiSegmentFace_CommittedStateWithoutMedia_DrawsInsteadOfThrowing`.
|
||||
Verified by reverting the one-line fix: the test throws `ArgumentNullException`
|
||||
from `UiButton.ActiveFile`, the same frame as the live crash.
|
||||
|
||||
**Note for whoever tidies this file:** the crash was found while
|
||||
investigating Campaign LU item 4 ("launching the selected character doesn't
|
||||
work"). It is why nothing worked — the client reached character select and
|
||||
died there. Distinct from the LU5 UX work.
|
||||
|
||||
## #419 — Portal-tunnel rim polygon visible (FOV-coupled) + ring flash at exit (camera dolly vs retail's view-plane animation)
|
||||
|
||||
**Status:** OPEN (filed 2026-08-17, user screenshot + FOV experiment).
|
||||
|
|
|
|||
262
docs/plans/2026-08-19-launcher-usability-campaign.md
Normal file
262
docs/plans/2026-08-19-launcher-usability-campaign.md
Normal file
|
|
@ -0,0 +1,262 @@
|
|||
# Campaign LU — launcher usability
|
||||
|
||||
**Status:** PLANNED 2026-08-19. Not started; awaiting the goal being set.
|
||||
|
||||
**Goal**
|
||||
|
||||
> The launcher opens without a long wait. On startup it asks whether to
|
||||
> update the launcher or the client, and restarts itself after a launcher
|
||||
> update; the old update flow is gone. First-run setup ends with a success
|
||||
> popup that returns you to the launcher on OK. A selected character
|
||||
> launches directly. The sessions frame shows account, character (or Char
|
||||
> Select) and whether they are in game — not the launch mode.
|
||||
|
||||
**Why now.** Campaign LA shipped a launcher that is *correct* — atomic
|
||||
installs, verified artifacts, session barriers, rollback — and *not
|
||||
usable*. The user's verdict, twice: "way too complex", "too complex for
|
||||
sending it to my friends". This campaign changes the surface a person
|
||||
touches. It does not weaken what happens underneath.
|
||||
|
||||
**Acceptance for the whole campaign** is the user's own walkthrough:
|
||||
download `launcher-win-x64.zip` from the `latest` release, unzip, run,
|
||||
install, play — without being told anything.
|
||||
|
||||
---
|
||||
|
||||
## LU1 — the launcher opens immediately
|
||||
|
||||
**Measured problem.** [App.axaml.cs:57](../../src/AcDream.Launcher/App.axaml.cs)
|
||||
blocks the UI thread on `installer.LoadExistingAsync().GetAwaiter().GetResult()`
|
||||
before the window is constructed. That reaches
|
||||
`LauncherInstallRecordStore.VerifyFileAsync`, which computes a full SHA-256
|
||||
of the installed package.
|
||||
|
||||
Measured on the user's machine 2026-08-19:
|
||||
|
||||
| fact | value |
|
||||
|---|---|
|
||||
| `%LOCALAPPDATA%\acdream\pak\acdream.pak` | 29,908,271,024 bytes (27.9 GiB) |
|
||||
| full SHA-256 | **24.1 s** at 1.16 GB/s |
|
||||
| digest vs `install.json` | identical (`fee8595d…`) |
|
||||
|
||||
So the startup cost is 24 s of disk read to re-confirm something that was
|
||||
already true. A friend does not see it only because they have no package
|
||||
installed yet — verification short-circuits at "nothing installed". It
|
||||
will hit them the moment first-run setup finishes.
|
||||
|
||||
**Change.** Startup verification becomes size + last-write-time against
|
||||
the record. The full hash keeps running where it is cheap and meaningful:
|
||||
at install, after an update installs a new package, and behind an explicit
|
||||
**Verify files** button (the Steam shape).
|
||||
|
||||
The cheap facts live in a **sidecar** (`install.verification.json`), not as a
|
||||
new field on the install record. `LauncherInstallRecordStore` reads
|
||||
`install.json` with `JsonUnmappedMemberHandling.Disallow`, so a new field
|
||||
there would make an *older* launcher build reject the record outright and
|
||||
demand a 28 GB re-bake after a rollback. An unknown sidecar file is simply
|
||||
ignored by older builds, so the change is compatible in both directions.
|
||||
An install with no sidecar yet pays one full hash and then writes it.
|
||||
|
||||
**Acceptance**
|
||||
|
||||
- Window visible in under 2 s with the 27.9 GiB package installed.
|
||||
- Truncating or touching the package still blocks launch with a clear reason.
|
||||
- **Verify files** reproduces the full check and reports pass/fail.
|
||||
- The install and update paths still hash in full — unchanged.
|
||||
|
||||
---
|
||||
|
||||
## LU2 — one update question, asked once, at startup
|
||||
|
||||
**Change.** On start the launcher checks the feed once. If the launcher or
|
||||
the client is behind, it shows **one** dialog naming what is out of date and
|
||||
offering **Update** / **Not now**. Nothing else.
|
||||
|
||||
- Launcher first when the feed's `minimumLauncherVersion` demands it, or
|
||||
when only the launcher is behind: install, then **restart into the new
|
||||
version** (`LauncherSelfUpdateBootstrap` already owns this handoff).
|
||||
- Client otherwise: install, close the dialog, back at the launcher.
|
||||
- Nothing to do: no dialog at all. The launcher just opens.
|
||||
|
||||
**Acceptance** — three observed cases: up to date (silent), client behind
|
||||
(one dialog → play), launcher behind (one dialog → relaunched on the new
|
||||
version, confirmed by the version it reports).
|
||||
|
||||
---
|
||||
|
||||
## LU3 — delete the old update surface
|
||||
|
||||
The current prompt offers six buttons — Check again, Rollback client,
|
||||
Stage launcher, Install client, Cancel, Close — plus a version table and a
|
||||
restart-required banner. That is the flow being removed, along with the
|
||||
"Check for updates" header button and the `LauncherUpdateViewModel` paths
|
||||
only it reached.
|
||||
|
||||
**What stays:** everything in `AcDream.Launcher.Core/Updates/` that makes
|
||||
an update safe — manifest validation, bounded verified download, safe ZIP
|
||||
extraction, versioned install with an atomic `current.json` switch, the
|
||||
session barrier, and rollback as a *capability*. The complexity the user
|
||||
objects to is the panel, not the safety beneath it.
|
||||
|
||||
**Open — needs one confirmation before code is deleted:** rollback has no
|
||||
place in the new single-question flow. It can move behind a small
|
||||
"Advanced" affordance or leave the UI entirely (staying available as Core
|
||||
API + tests). I will show the exact deletion list and ask before removing
|
||||
it.
|
||||
|
||||
**Acceptance** — exactly one update entry point in the UI; tests covering
|
||||
deleted view-model behavior are removed with the code, never skipped.
|
||||
|
||||
---
|
||||
|
||||
## LU4 — "Setup complete" ends first-run setup
|
||||
|
||||
**Change.** When the bake publishes and the install record verifies, the
|
||||
wizard shows a modal: setup succeeded, what was built, **OK**. OK closes
|
||||
the wizard and returns to the launcher with the "Client setup required"
|
||||
banner gone and launching enabled.
|
||||
|
||||
**Acceptance** — a real first-run bake shows it exactly once on success;
|
||||
cancellation and failure paths keep their existing error/status reporting
|
||||
and must **not** show it.
|
||||
|
||||
---
|
||||
|
||||
## LU5 — pressing Play on a character launches that character
|
||||
|
||||
**Reproduce before changing anything.** The plumbing already exists end to
|
||||
end: `LauncherOrchestrator.LaunchAsync` clones the character with the
|
||||
*requested* mode (`CloneCharacter(character, mode)`),
|
||||
`SessionConfigComposer.BuildSelector` emits an id selector (falling back to
|
||||
name), and `RuntimeOptions.MapCharacterSelector` maps it into the App host.
|
||||
A defect somewhere in a chain that reads correct is exactly the case this
|
||||
project has repeatedly lost time to by guessing.
|
||||
|
||||
Two candidates to separate by observation, not argument:
|
||||
|
||||
1. The launch button is gated off by a capability reason, so the click
|
||||
never becomes a session.
|
||||
2. The selector reaches the client but the roster match fails, so character
|
||||
select stays on screen — which is what "you can just select different
|
||||
chars" describes.
|
||||
|
||||
**Change.** One obvious **Play** per character that enters the world as
|
||||
that character, plus the deliberate "Character select" path kept separate.
|
||||
Three near-identical launch buttons is itself part of the complaint.
|
||||
|
||||
**Acceptance** — select a character, press Play, arrive in the world as
|
||||
that character with no character-select screen in between.
|
||||
|
||||
---
|
||||
|
||||
## LU6 — the sessions frame says who is playing
|
||||
|
||||
Today each row reads `server / account / character`, then `Mode`
|
||||
(Gui/GuiSelect/Headless/Probe), then `State`, then a raw status string.
|
||||
The launch mode is launcher bookkeeping and means nothing to a player.
|
||||
|
||||
**Change.** Each row shows the account, the character — or **Character
|
||||
select** when no character was chosen — and one plain status word derived
|
||||
from the host's own status stream:
|
||||
|
||||
`Starting` → `Character select` → `In game` → `Stopped` / `Failed`
|
||||
|
||||
Errors keep their own line. Stop keeps its button. Character-refresh
|
||||
(probe) rows stay distinguishable from play sessions.
|
||||
|
||||
**Acceptance** — launching a character shows account + name + **In game**
|
||||
once in world; a character-select launch shows **Character select** until a
|
||||
character is entered.
|
||||
|
||||
---
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No change to download verification, atomic install, or the session barrier.
|
||||
- No change to credential handling (plaintext profile remains the user's decision).
|
||||
- No change to Linux graphical gating (Slice L stays parked).
|
||||
|
||||
## Working rules for this campaign
|
||||
|
||||
- One slice per commit, `dotnet build` + `dotnet test` green before each.
|
||||
- Push to main; CI gates on both runners and publishes the release the
|
||||
launcher itself updates from — so every slice is testable by the user
|
||||
through the shipped path within a few minutes.
|
||||
- LU3's deletions and LU5's root cause get shown to the user before they
|
||||
land.
|
||||
|
||||
---
|
||||
|
||||
# Implementation notes (recon 2026-08-19, before any code)
|
||||
|
||||
These were read out of the tree, not assumed. They exist so each slice
|
||||
starts from the mechanism that is already there instead of re-deriving it.
|
||||
|
||||
## The self-update restart chain already exists end to end (LU2)
|
||||
|
||||
`LauncherUpdater.StageLauncherAsync` stages a verified payload and writes a
|
||||
plan. On the next ordinary startup `LauncherSelfUpdateBootstrap.HandleAsync`
|
||||
takes the exclusive lease, sees `SelfUpdatePlanState.Staged`, and spawns the
|
||||
STAGED launcher in helper mode. `RunHelperAsync` waits for the parent PID to
|
||||
exit, applies the replacement, starts the updated launcher with
|
||||
`--acdream-self-update-confirm-v1`, and waits for the confirmation receipt.
|
||||
|
||||
So "restart after a launcher update" needs no new update machinery. What it
|
||||
needs is one seam: after staging succeeds, start the staged helper against
|
||||
the CURRENT process and shut down. Extract the existing staged-plan branch of
|
||||
`HandleAsync` into a callable entry point and reuse it — do not duplicate it,
|
||||
and do not restart by launching a second copy of the launcher and hoping the
|
||||
bootstrap picks the plan up, which races the exclusive lease against the
|
||||
process that is still shutting down.
|
||||
|
||||
## The orchestrator already knows "in game" (LU6)
|
||||
|
||||
`LauncherActivityState` has `InWorld`, and the orchestrator already sets it
|
||||
from `EnteredWorldStatusEvent`, which carries the real `CharacterId` and
|
||||
`CharacterName` from the host. Today that identity is written into a status
|
||||
STRING (`"In world as X."`) and thrown away.
|
||||
|
||||
LU6 promotes it: the entered-world event updates the activity's character
|
||||
name so a character-select launch can show who is actually being played, and
|
||||
the row renders one word derived from `LauncherActivityState` rather than the
|
||||
raw enum plus the launch mode:
|
||||
|
||||
| state | row shows |
|
||||
|---|---|
|
||||
| `Starting`, `Running` | Starting |
|
||||
| `Connected` | Character select |
|
||||
| `InWorld` | In game |
|
||||
| `Disconnected`, `Stopping` | Stopping |
|
||||
| `Exited`, `Cancelled` | Stopped |
|
||||
| `Failed` | Failed |
|
||||
|
||||
`LauncherActivityKind.Probe` rows stay visually distinct (they are a
|
||||
character refresh, not a play session).
|
||||
|
||||
## First-run completion has an exact point (LU4)
|
||||
|
||||
`FirstRunInstallerViewModel.StartAsync` succeeds at the line that calls
|
||||
`_onInstalled(result.Record)` and sets `Phase = LauncherInstallPhase.Completed`.
|
||||
That is where the success dialog belongs — after the record is published, so
|
||||
the launcher behind it is already in its launch-enabled state when the user
|
||||
presses OK. The cancelled and failed branches immediately below it must not
|
||||
reach it.
|
||||
|
||||
## The launcher side of "launch this character" reads correct (LU5)
|
||||
|
||||
Confirmed by reading, so the live repro can skip re-checking these:
|
||||
|
||||
- `LauncherOrchestrator.LaunchAsync` -> `CloneCharacter(character, mode)`
|
||||
overrides the profile's saved `LaunchMode` with the mode the button asked
|
||||
for, so the stored default cannot leak into an explicit launch.
|
||||
- `SessionConfigComposer.Compose` builds a selector for every mode except
|
||||
`GuiSelect`, preferring a parsed non-zero id over the name.
|
||||
- `SessionPlayerComposition` passes the selector into
|
||||
`LiveSessionConnectOptions` with `AwaitCharacterSelection: selector is null`,
|
||||
and `InteractionRetainedUiComposition` binds the character-selection UI only
|
||||
when the selector is null.
|
||||
|
||||
The user's stored profiles all carry `launchMode: "guiSelect"` (the default),
|
||||
and every cached character has a real id. So the defect is NOT a missing id
|
||||
and NOT the saved default overriding the click. Reproduce live before
|
||||
changing anything.
|
||||
|
|
@ -441,6 +441,19 @@ public sealed class UiButton : UiElement, IUiGlobalTimeListener, IUiDatStateful
|
|||
// state at construction, exactly retail's Initialize -> SetState
|
||||
// ordering.
|
||||
_segmentMediaStates = new string[_faceSegments.Length];
|
||||
// #420: seed every segment with DirectState (""), exactly like
|
||||
// _faceMediaState's own initializer above. `new string[n]` leaves
|
||||
// nulls, and NextMediaState returns `current` UNCHANGED on three of
|
||||
// its four arms (committed state authored with an empty media array,
|
||||
// or no committed/base state and no "" entry) — so on a multi-segment
|
||||
// button whose committed state carries no media the null survived the
|
||||
// first SyncMediaStates and reached
|
||||
// ElementInfo.StateMedia.TryGetValue(null), throwing
|
||||
// ArgumentNullException ("Parameter 'key'") from inside OnDraw.
|
||||
// Observed live: it killed the client mid-paint on the character-
|
||||
// select screen on every launch (session status.jsonl: connected ->
|
||||
// characterList -> exited code 1 "crashed").
|
||||
Array.Fill(_segmentMediaStates, "");
|
||||
_resolve = resolve;
|
||||
ClickThrough = false; // buttons are interactive — opt OUT of click-through
|
||||
|
||||
|
|
|
|||
|
|
@ -745,6 +745,35 @@ public class UiButtonTests
|
|||
private static UiButton CreateButton(ElementInfo info)
|
||||
=> new(info, NoTex) { Width = info.Width, Height = info.Height };
|
||||
|
||||
/// <summary>
|
||||
/// #420 regression. A multi-segment face whose committed state authors no
|
||||
/// media used to leave that segment's media-state name NULL (the array
|
||||
/// started as <c>new string[n]</c> and NextMediaState returns the previous
|
||||
/// value unchanged on that arm), and the null then reached
|
||||
/// <c>ElementInfo.StateMedia.TryGetValue</c>, throwing
|
||||
/// ArgumentNullException from inside OnDraw. Live symptom: the client
|
||||
/// crashed on the character-select screen on every launch.
|
||||
///
|
||||
/// <para>The assertion is secondary — the point is that drawing COMPLETES.
|
||||
/// Before the fix this test throws instead of failing.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void MultiSegmentFace_CommittedStateWithoutMedia_DrawsInsteadOfThrowing()
|
||||
{
|
||||
var info = new ElementInfo { Type = 1, Width = 32, Height = 16 };
|
||||
var button = new UiButton(
|
||||
info,
|
||||
static file => (file, 8, 8),
|
||||
mediaInfo: null,
|
||||
faceSegments: [new ElementInfo { Type = 1, Width = 16, Height = 16 }])
|
||||
{
|
||||
Width = info.Width,
|
||||
Height = info.Height,
|
||||
};
|
||||
|
||||
Assert.Equal(0u, DrawnFaceFile(button));
|
||||
}
|
||||
|
||||
// ── #416 media-rule draw harness ─────────────────────────────────────
|
||||
|
||||
private sealed class NullGpuFrameSource
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue