diff --git a/docs/ISSUES.md b/docs/ISSUES.md index b38196dd..240c0622 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -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). diff --git a/docs/plans/2026-08-19-launcher-usability-campaign.md b/docs/plans/2026-08-19-launcher-usability-campaign.md new file mode 100644 index 00000000..c55026e3 --- /dev/null +++ b/docs/plans/2026-08-19-launcher-usability-campaign.md @@ -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. diff --git a/src/AcDream.App/UI/UiButton.cs b/src/AcDream.App/UI/UiButton.cs index bdba5f1d..c5fd0d21 100644 --- a/src/AcDream.App/UI/UiButton.cs +++ b/src/AcDream.App/UI/UiButton.cs @@ -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 diff --git a/tests/AcDream.App.Tests/UI/UiButtonTests.cs b/tests/AcDream.App.Tests/UI/UiButtonTests.cs index f124991f..aba80c27 100644 --- a/tests/AcDream.App.Tests/UI/UiButtonTests.cs +++ b/tests/AcDream.App.Tests/UI/UiButtonTests.cs @@ -745,6 +745,35 @@ public class UiButtonTests private static UiButton CreateButton(ElementInfo info) => new(info, NoTex) { Width = info.Width, Height = info.Height }; + /// + /// #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 new string[n] and NextMediaState returns the previous + /// value unchanged on that arm), and the null then reached + /// ElementInfo.StateMedia.TryGetValue, throwing + /// ArgumentNullException from inside OnDraw. Live symptom: the client + /// crashed on the character-select screen on every launch. + /// + /// The assertion is secondary — the point is that drawing COMPLETES. + /// Before the fix this test throws instead of failing. + /// + [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