fix(chargen): Campaign CC CC5 review fix round — F1-F14
Opus dual-lens review of 34e3a534+a975efd1 returned architectural PASS-with-items / retail-fidelity FAIL. Every finding fixed: - F1 (BLOCKER): deleted CharacterCreationSummaryPage's dead _suppressNextFieldEvent latch. UiField.SetText never raises OnFocusLost/OnSubmit, so the latch never had anything genuine to suppress — it stayed armed until the player's own next real commit and silently ate their typed name. - F2: byte-re-derived gmCharGenMainUI::RecvNotice_ CharGenVerificationResponse @0x004e9030's jump table — Pending is an explicit switch case landing on the SAME NameDBDown label as Corrupt/DatabaseDown, and Undef/out-of-range falls through the function's own unsigned-underflow default arm to that identical label. Retail's dispatch has NO silent branch. ApplyCreationResponse now produces a real rejection for Pending/Undef instead of a silent reset; ReconcileDialogs maps them to NameDBDown. Corrects the wrong "retail swallows Pending" claim everywhere it was repeated (plan doc, Core.Net doc comment, Runtime doc comments). - F3: skill rows now use the key/value template with CharGenState::GetSkillScore @0x005C4B50 as the value (ported via the new RetailSkillFormula.CalculateChargenScore / ChargenSkillScoreResolver, wired through a new GetSkillScore binding), not template 0/name-only; bucket headers are unconditional. Writing this fix's own regression test surfaced a second, more severe bug: CharacterCreationSummaryPage never wired _list.TemplateResolver at all, so RebuildListbox has been a silent no-op since CC5 shipped — fixed by threading templateResolver through the page's constructor, matching every sibling UiTemplateListBox owner. - F4: added the missing _errorMessageDialogContext one-outstanding guard to the 0xF643 rejection dialog, matching MakeErrorMessageDialog's own guard @0x004e8cc4 and the other four sibling dialogs' shape (registered in CloseAllDialogs, suppress- callback checked). - F5: the Summary preview camera now seeds/re-derives retail's zoomed-OUT eye (byte-decoded (0,-2.5,0.95) at gmCGSummaryPage:: InitializePage ~0x0047bd14-0x0047bd44) instead of Appearance's zoomed-in default, via a new ChargenPreviewController useZoomedOutEye flag. - F6: retired AP-225 outright — re-derived the ListenToElementMessage length gate is NUL-inclusive, so MaxNameLength=32 was always byte-correct, not merely internally consistent. - F7: amended AP-221 to cover the Summary preview's duplicate one-shot-composition binding gap (CC5 duplicated the pattern instead of closing it). - F8: byte-decoded GetRandomReal @0x00563940's fmul operand at 0x007cd650 — an 8-byte double, not a 4-byte float — is EXACTLY 1.0/32767.0, not 1/32768. Added RollShadeLocked (_random.Next(32768) * (1.0/32767.0)) and switched all six shade rolls onto it. - F9: evaluated porting retail's exact empty-name-commit no-op (NUL-inclusive length==1 skips SetName entirely) and rejected it — it would fight the F1 field-sync model by spontaneously reverting an emptied field on the next unrelated revision bump. Kept the clear, documented the tradeoff, filed AP-227. - F11: filed AP-226 documenting retail's static pcProfessions/pcGender/ pcHeritage/pcTown label tables versus acdream's DAT-sourced labels, including the non-human-heritage-renders-bare-"Heritage:" retail quirk. - F12: added exclude-current determinism (count-2 lists), Random- clears-name, repeat-identical-rejection-reshows, and RebuildListbox content tests (the last one found F3's TemplateResolver bug). - F13: threaded an optional Random through GameRuntimeDependencies -> LiveSessionController -> RuntimeCharacterCreationState, matching the existing TimeProvider injection shape, closing the Slice-K determinism hazard on a bot-reachable Randomize* command family. - F14: RandomizeCharacterLocked now assigns _heritageId unconditionally before the TryGetHeritage gate, matching retail's SetHeritageGroup @0x005C67A0 (mHeritageGroup written before the DAT lookup). Gates: Runtime 1726/0 (was 1722/0), App 5242/3 skips (was 5240/3), Headless 166/0, Core.Net 993/994 (the one failure, NakEmissionTests LossSoak, is a known pre-existing flake — passes standalone), full solution Release build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
a975efd1d5
commit
0c8e1e7df1
14 changed files with 720 additions and 141 deletions
|
|
@ -19,7 +19,18 @@ public sealed record GameRuntimeDependencies(
|
|||
ILiveSessionOperations? SessionOperations = null,
|
||||
Func<double>? CombatTime = null,
|
||||
uint FirstLocalEntityId = RuntimeEntityDirectory.FirstLocalEntityId,
|
||||
int MaximumChatEntries = 500);
|
||||
int MaximumChatEntries = 500,
|
||||
// Review fix round F13 (2026-08-16): the shared RNG source for
|
||||
// process-wide randomize commands reachable from a headless bot
|
||||
// (RuntimeCharacterCreationState's Randomize* family —
|
||||
// RandomizeCharacter/RandomizeAppearance/RandomizeClothing, exposed on
|
||||
// IRuntimeCharacterCreationCommands). null (the default) keeps every
|
||||
// existing caller's production behavior unchanged (Random.Shared,
|
||||
// threaded the same way TimeProvider already is here) — this only
|
||||
// exists so a future deterministic-bot config (Slice K's contract,
|
||||
// project_linux_headless_bots.md) can supply a seeded Random without a
|
||||
// second construction path.
|
||||
Random? Random = null);
|
||||
|
||||
[Flags]
|
||||
public enum GameRuntimeTeardownStage
|
||||
|
|
@ -180,10 +191,12 @@ public sealed class GameRuntime
|
|||
context.Session = dependencies.SessionOperations is null
|
||||
? new LiveSessionController(
|
||||
ProductionLiveSessionOperations.Instance,
|
||||
dependencies.TimeProvider)
|
||||
dependencies.TimeProvider,
|
||||
random: dependencies.Random)
|
||||
: new LiveSessionController(
|
||||
dependencies.SessionOperations,
|
||||
dependencies.TimeProvider);
|
||||
dependencies.TimeProvider,
|
||||
random: dependencies.Random);
|
||||
construction.Own(context.Session);
|
||||
Fault(
|
||||
GameRuntimeConstructionPoint.SessionCreated,
|
||||
|
|
|
|||
|
|
@ -455,7 +455,13 @@ public sealed class LiveSessionController
|
|||
public LiveSessionController(
|
||||
ILiveSessionOperations operations,
|
||||
TimeProvider? timeProvider = null,
|
||||
ChargenOptions? chargenOptions = null)
|
||||
ChargenOptions? chargenOptions = null,
|
||||
// Review fix round F13 (2026-08-16): threaded from
|
||||
// GameRuntimeDependencies.Random the same way timeProvider already
|
||||
// is — null keeps every existing caller (including this class's own
|
||||
// parameterless ctor below) on RuntimeCharacterCreationState's own
|
||||
// Random.Shared default.
|
||||
Random? random = null)
|
||||
{
|
||||
_operations = operations ?? throw new ArgumentNullException(nameof(operations));
|
||||
CharacterSelectionState = new RuntimeCharacterSelectionState(
|
||||
|
|
@ -466,7 +472,7 @@ public sealed class LiveSessionController
|
|||
// not this one. A caller that never supplies real options simply
|
||||
// gets an inert chargen surface (every heritage lookup misses).
|
||||
CharacterCreationState = new RuntimeCharacterCreationState(
|
||||
chargenOptions ?? ChargenOptions.Empty);
|
||||
chargenOptions ?? ChargenOptions.Empty, random);
|
||||
}
|
||||
|
||||
public RuntimeCharacterSelectionState CharacterSelectionState { get; }
|
||||
|
|
|
|||
|
|
@ -163,18 +163,25 @@ public readonly record struct RuntimeCharacterCreationIdentity(
|
|||
|
||||
/// <summary>
|
||||
/// A non-Ok <c>0xF643</c> response, mapped to retail's dialog family
|
||||
/// (<c>Handle_CharGenVerificationResponse @ 0x0055E8B0</c>'s per-case dialog
|
||||
/// dispatch, restated in <see cref="CharGenVerificationResponse"/>'s doc
|
||||
/// comment): <see cref="CharGenVerificationResponse.Code.NameInUse"/> →
|
||||
/// NameReserved, <see cref="CharGenVerificationResponse.Code.NameBanned"/> →
|
||||
/// NameBanned, <see cref="CharGenVerificationResponse.Code.Corrupt"/> /
|
||||
/// <see cref="CharGenVerificationResponse.Code.DatabaseDown"/> → NameDBDown,
|
||||
/// <see cref="CharGenVerificationResponse.Code.AdminPrivilegeDenied"/> →
|
||||
/// NameAdminDenied. <see cref="CharGenVerificationResponse.Code.Pending"/> /
|
||||
/// <see cref="CharGenVerificationResponse.Code.Undef"/> never produce this
|
||||
/// record — retail treats them as a silent state reset with no dialog (ACE
|
||||
/// sends <c>Pending</c> for a disabled-Olthoi rejection; this is a genuine
|
||||
/// retail quirk, not a bug — port as-is).
|
||||
/// (<c>Handle_CharGenVerificationResponse @ 0x0055E8B0</c> +
|
||||
/// <c>gmCharGenMainUI::RecvNotice_CharGenVerificationResponse @
|
||||
/// 0x004e9030</c>'s own switch/jump-table dispatch, restated in
|
||||
/// <see cref="CharGenVerificationResponse"/>'s doc comment).
|
||||
/// <b>CC5 review-fix round F2 (2026-08-16) correction:</b> ALL non-Ok codes
|
||||
/// produce this record — <see cref="CharGenVerificationResponse.Code.NameInUse"/>
|
||||
/// → NameReserved, <see cref="CharGenVerificationResponse.Code.NameBanned"/> →
|
||||
/// NameBanned, <see cref="CharGenVerificationResponse.Code.AdminPrivilegeDenied"/> →
|
||||
/// NameAdminDenied, and <see cref="CharGenVerificationResponse.Code.Pending"/> /
|
||||
/// <see cref="CharGenVerificationResponse.Code.Corrupt"/> /
|
||||
/// <see cref="CharGenVerificationResponse.Code.DatabaseDown"/> /
|
||||
/// <see cref="CharGenVerificationResponse.Code.Undef"/> / any unrecognized
|
||||
/// code ALL → NameDBDown (Pending is an explicit switch case landing on
|
||||
/// that same label; Undef/out-of-range falls through the function's own
|
||||
/// unsigned-underflow default arm to the identical label). An earlier
|
||||
/// reading of the decomp treated Pending/Undef as producing a silent state
|
||||
/// reset with NO record and no dialog — that was wrong; retail's dispatch
|
||||
/// has no silent branch (ACE sends <c>Pending</c> for a disabled-Olthoi
|
||||
/// rejection, which now correctly surfaces the NameDBDown dialog).
|
||||
/// </summary>
|
||||
public readonly record struct RuntimeCharacterCreationRejection(
|
||||
uint RawCode,
|
||||
|
|
@ -1253,6 +1260,27 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
return result;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Ports <c>CharGenState::GetRandomReal @ 0x00563940</c> exactly:
|
||||
/// <c>(double)rand() * (1.0/32767.0)</c>. Review fix round F8
|
||||
/// (2026-08-16), byte-decoded from the raw PE: the pseudo-C shows only
|
||||
/// <c>return rand(this);</c> (the decompiler elided the FPU multiply
|
||||
/// entirely), but the actual machine code is
|
||||
/// <c>call rand; fild [esp]; fmul qword ptr [0x007cd650]; ret</c> — an
|
||||
/// 8-BYTE double-precision operand (<c>fmul qword</c>, not <c>dword</c>).
|
||||
/// The bytes at <c>0x007cd650</c> are <c>80 00 40 00 20 00 00 3f</c>,
|
||||
/// which as a little-endian IEEE-754 double is EXACTLY
|
||||
/// <c>1.0/32767.0</c> (bit pattern <c>0x3f00002000400080</c>) — NOT
|
||||
/// <c>1.0/32768.0</c> (which would be <c>0x3f00000000000000</c>), a
|
||||
/// prior narrower reading corrects here. Retail's CRT <c>rand()</c>
|
||||
/// returns <c>[0, RAND_MAX]</c> with <c>RAND_MAX == 0x7FFF == 32767</c>
|
||||
/// (MSVC), so the shade roll is a 32768-point lattice on
|
||||
/// <c>[0.0, 1.0]</c> INCLUSIVE (both endpoints reachable) —
|
||||
/// <see cref="Random.NextDouble()"/>'s continuous <c>[0, 1)</c> is a
|
||||
/// different distribution entirely.
|
||||
/// </summary>
|
||||
private double RollShadeLocked() => _random.Next(32768) * (1.0 / 32767.0);
|
||||
|
||||
/// <summary>Ports <c>CharGenState::RandomizeAppearance(this, 0) @
|
||||
/// 0x005c4f10</c> — every real call site in the retail binary passes
|
||||
/// <c>arg2 == 0</c> (an exhaustive grep of every <c>RandomizeAppearance</c>
|
||||
|
|
@ -1261,8 +1289,8 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
/// code and is not ported. Each field is only rolled when its list is
|
||||
/// non-empty (retail's own per-field <c>if (count != 0)</c> guards);
|
||||
/// <c>skinShade</c>/<c>hairShade</c> are <c>vtable->GetRandomReal()</c>
|
||||
/// — the SAME <c>rand()*(1/32768)</c> uniform-[0,1) shade roll every
|
||||
/// other Randomize* function below uses explicitly inline.</summary>
|
||||
/// — the SAME <see cref="RollShadeLocked"/> shade roll every other
|
||||
/// Randomize* function below uses.</summary>
|
||||
private void RandomizeAppearanceLocked()
|
||||
{
|
||||
if (!TryGetGenderOptionsLocked(out ChargenGenderOptions? gender))
|
||||
|
|
@ -1275,7 +1303,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
a = a with { NoseStrip = RandomizeIndexExcludingLocked(gender.NoseStrips.Count, a.NoseStrip) };
|
||||
if (gender.MouthStrips.Count > 0)
|
||||
a = a with { MouthStrip = RandomizeIndexExcludingLocked(gender.MouthStrips.Count, a.MouthStrip) };
|
||||
a = a with { SkinShade = _random.NextDouble(), HairShade = _random.NextDouble() };
|
||||
a = a with { SkinShade = RollShadeLocked(), HairShade = RollShadeLocked() };
|
||||
if (gender.HairColors.Count > 0)
|
||||
a = a with { HairColor = RandomizeIndexExcludingLocked(gender.HairColors.Count, a.HairColor) };
|
||||
if (gender.EyeColors.Count > 0)
|
||||
|
|
@ -1328,7 +1356,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
HeadgearColor = RandomizeIndexExcludingLocked(colorCount, _appearance.HeadgearColor),
|
||||
};
|
||||
}
|
||||
_appearance = _appearance with { HeadgearShade = _random.NextDouble() };
|
||||
_appearance = _appearance with { HeadgearShade = RollShadeLocked() };
|
||||
}
|
||||
|
||||
/// <summary>Ports <c>CharGenState::RandomizeShirt @ 0x005c5ef0</c> —
|
||||
|
|
@ -1354,7 +1382,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
ShirtColor = RandomizeIndexExcludingLocked(colorCount, _appearance.ShirtColor),
|
||||
};
|
||||
}
|
||||
_appearance = _appearance with { ShirtShade = _random.NextDouble() };
|
||||
_appearance = _appearance with { ShirtShade = RollShadeLocked() };
|
||||
}
|
||||
|
||||
/// <summary>Ports <c>CharGenState::RandomizeTrousers @ 0x005c5fb0</c>.</summary>
|
||||
|
|
@ -1378,7 +1406,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
TrousersColor = RandomizeIndexExcludingLocked(colorCount, _appearance.TrousersColor),
|
||||
};
|
||||
}
|
||||
_appearance = _appearance with { TrousersShade = _random.NextDouble() };
|
||||
_appearance = _appearance with { TrousersShade = RollShadeLocked() };
|
||||
}
|
||||
|
||||
/// <summary>Ports <c>CharGenState::RandomizeFootwear @ 0x005c6070</c>.</summary>
|
||||
|
|
@ -1402,7 +1430,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
FootwearColor = RandomizeIndexExcludingLocked(colorCount, _appearance.FootwearColor),
|
||||
};
|
||||
}
|
||||
_appearance = _appearance with { FootwearShade = _random.NextDouble() };
|
||||
_appearance = _appearance with { FootwearShade = RollShadeLocked() };
|
||||
}
|
||||
|
||||
/// <summary>Ports <c>CharGenState::RandomizeClothing(this, arg2) @
|
||||
|
|
@ -1486,6 +1514,18 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
ClearSessionState();
|
||||
|
||||
uint heritageId = (uint)RollDiceLocked(1, 4);
|
||||
// Review fix round F14 (2026-08-16): retail's SetHeritageGroup @
|
||||
// 0x005C67A0 writes `this->mHeritageGroup = arg2;` UNCONDITIONALLY
|
||||
// as its very first statement, before the DAT lookup
|
||||
// (ACCharGenData::GetHG) that gates the credit/template/start-area
|
||||
// recompute. Assign the raw field here too, before the
|
||||
// TryGetHeritage gate below, so a hypothetical DAT-lookup miss
|
||||
// leaves `_heritageId` set (matching retail's unconditional write)
|
||||
// instead of the previous half-state where heritage stayed 0 while
|
||||
// SetGenderLocked below still ran and produced a real gender.
|
||||
// Unreachable with the installed DAT — every rolled id 1..4 always
|
||||
// resolves — this is defensive shape-parity only.
|
||||
_heritageId = heritageId;
|
||||
if (_options.TryGetHeritage(heritageId, out ChargenHeritageOptions? heritage))
|
||||
SetHeritageGroupLocked(heritageId, heritage);
|
||||
|
||||
|
|
@ -1765,11 +1805,23 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
}
|
||||
|
||||
/// <summary>
|
||||
/// Ports the four rejection dialog mappings + the silent
|
||||
/// Pending/Undef reset from <c>Handle_CharGenVerificationResponse @
|
||||
/// 0x0055E8B0</c>. Idempotent-tolerant to a second, unrequested Ok/reject
|
||||
/// while nothing is pending (ACE's own double-NameInUse quirk, CC2
|
||||
/// review F3) — a call that arrives while
|
||||
/// Ports the full rejection-dialog dispatch from
|
||||
/// <c>Handle_CharGenVerificationResponse @ 0x0055E8B0</c> +
|
||||
/// <c>gmCharGenMainUI::RecvNotice_CharGenVerificationResponse @
|
||||
/// 0x004e9030</c>'s own jump table. CC5 review-fix round F2
|
||||
/// (2026-08-16): every non-Ok code produces a
|
||||
/// <see cref="RuntimeCharacterCreationRejection"/> — there is no silent
|
||||
/// branch. Pending previously short-circuited as a bare state reset
|
||||
/// with no rejection produced; that was a misreading of the decomp
|
||||
/// (Pending is an explicit switch case in
|
||||
/// <c>RecvNotice_CharGenVerificationResponse</c> landing on the SAME
|
||||
/// <c>ID_Character_Err_NameDBDown</c> label as Corrupt/DatabaseDown,
|
||||
/// and Undef/any out-of-range code falls through that function's own
|
||||
/// <c>(arg2-1) > 6</c> unsigned-underflow default arm to the
|
||||
/// identical label) — see the <c>else</c> branch's own inline comment
|
||||
/// for the full citation. Idempotent-tolerant to a second, unrequested
|
||||
/// Ok/reject while nothing is pending (ACE's own double-NameInUse
|
||||
/// quirk, CC2 review F3) — a call that arrives while
|
||||
/// <see cref="RuntimeCharacterCreationSnapshot.VerificationPending"/> is
|
||||
/// already false is a no-op rather than a second event.
|
||||
///
|
||||
|
|
@ -1777,13 +1829,7 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
/// Campaign CC slice CC3 review-fix round (F6): every branch below only
|
||||
/// SETS <c>kind</c> inside <c>lock (_gate)</c>; the single
|
||||
/// <see cref="Publish"/> call happens once, after the lock releases —
|
||||
/// matching every other public method in this class. The Pending/Undef
|
||||
/// branch previously published from inside the lock (harmless on its
|
||||
/// own — <see cref="Publish"/>'s own <c>lock (_gate)</c> is reentrant on
|
||||
/// the same thread — but inconsistent with the rest of the class and a
|
||||
/// lock-ordering risk once an observer callback reaches back into
|
||||
/// caller-held locks, e.g. <c>LiveSessionController._gate</c>, while
|
||||
/// still inside this one).
|
||||
/// matching every other public method in this class.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
internal void ApplyCreationResponse(CharGenVerificationResponse.Parsed response)
|
||||
|
|
@ -1803,15 +1849,29 @@ public sealed class RuntimeCharacterCreationState : IDisposable
|
|||
_lastRejection = null;
|
||||
kind = RuntimeCharacterCreationDeltaKind.Created;
|
||||
}
|
||||
else if (response.AsCode is CharGenVerificationResponse.Code.Pending
|
||||
or CharGenVerificationResponse.Code.Undef)
|
||||
{
|
||||
// Silent state reset — retail shows no dialog (ACE sends
|
||||
// Pending for a disabled-Olthoi rejection; port as-is).
|
||||
kind = RuntimeCharacterCreationDeltaKind.StateChanged;
|
||||
}
|
||||
else
|
||||
{
|
||||
// Review fix round F2 (2026-08-16): Pending/Undef used to
|
||||
// short-circuit here as a silent state reset with no
|
||||
// rejection produced — that was WRONG. Byte-decoded
|
||||
// gmCharGenMainUI::RecvNotice_CharGenVerificationResponse
|
||||
// @0x004e9030's own dispatch: Pending(2) is an explicit
|
||||
// switch case landing on the SAME "ID_Character_Err_
|
||||
// NameDBDown" label as Corrupt/DatabaseDown, and Undef(0)
|
||||
// (plus any code outside 1..7) falls through the function's
|
||||
// own "(arg2-1) > 6" unsigned-underflow default arm to that
|
||||
// identical label — retail's dispatch has NO silent branch
|
||||
// at all; every non-Ok code shows a dialog. Falling through
|
||||
// to this general rejection branch (instead of a special
|
||||
// silent-reset arm) now produces a real
|
||||
// RuntimeCharacterCreationRejection for Pending/Undef too,
|
||||
// which CharacterCreationUiController.ReconcileDialogs maps
|
||||
// to that same NameDBDown dialog (see its own doc comment).
|
||||
// Concrete effect: ACE's disabled-Olthoi Pending rejection
|
||||
// (CharacterHandler.CharacterCreateEx's olthoi_play_disabled
|
||||
// branch) now surfaces a visible dialog instead of silently
|
||||
// resetting verification state with Finish becoming a
|
||||
// permanent no-op.
|
||||
string reason = response.AsCode.ToString();
|
||||
_lastRejection = new RuntimeCharacterCreationRejection(
|
||||
response.RawCode,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue