fix(chargen): Campaign CC CC6b-MOUNT review fix round — F1-F13
Fixes every finding from the dual-lens review of 34c6fceab0 (architectural
PASS-with-items, retail-fidelity FAIL). Re-derived every decomp citation
against docs/research/named-retail/acclient_2013_pseudo_c.txt directly
rather than trusting the reviewer's transcription.
Wrap/normalize semantics (F1): CycleIndex's decrement-from-Unset landed on
0; the decomp's shared decrement tail (label_47f065/label_47f6d9, the same
switch the headgear ring was ported from) computes new=cur-1=-2 on the raw
signed int32, which wraps to count-1 — matching headgear's own ring shape.
Also ports the spin body-click normalize-and-write-back retail's cases
0xa5-0xae all share (NormalizeChoiceOnSelect), which acdream had dropped
entirely. Flips the one test that pinned the wrong expectation and adds
select-zone coverage no prior test isolated.
Heritage gate (F3): Update's Gearknight/Olthoi/OlthoiAcid branches reset
SetChoice(FACE)/SetSelection(HAIR) unconditionally, not only when Clothes
was showing — a conditional gate stranded Nose/Mouth as the current part
under a Face-tab session.
Doc corrections propagated everywhere they repeated (F4, F5, F6, plus the
plan doc's own CC6b-MOUNT ledger row for F1/F3): the gmBarberUI heading
citation conflated PostInit with InitializePage; Random's Appearance
disable was mislabeled a placeholder when it's really AP-212's unported
RandomizeAppearance/RandomizeClothing gap; the master-page doc still called
the Appearance page content-inert after this campaign made it real.
Visual substitutions widened (F2): AP-215 named only two of the Appearance
page's swatch/spin substitutions. Ports the two cheap ones directly —
current-part highlight via SetSelection's SetState(1)/SetState(6), routed
through the existing UiButtonStateMachine.Normal/Highlight ids and
IUiDatStateful.TrySetRetailState seam (installed-DAT-confirmed
ToggleBehavior=true on all nine spins); the shade scrollbar's SetVisible(0)
for Eyes vs acdream's Enabled=false. Files the other five (DoColorSpots,
the inert GradCircle, spin-caption/heritage-caption loss, the Skin-spin
MoveTo reposition, the Gearknight-boundary randomize calls) as new register
rows AP-216..AP-220 and corrects the plan doc's false claim that AP-215
already named the GradCircle.
Unlocked DAT read (F7, BLOCKER): ChargenPreviewController.Rebuild called
ChargenAppearanceFactory.TryCompose outside _datLock while the very next
line correctly locked TryBuildAnimated — CC6a's own F4 class of bug,
reintroduced at this catalog's first production call site. Wrapped in the
same lock; documented the invariant on ChargenAppearanceCatalog itself.
One-shot preview mount (F8): LivePresentationComposition reads
ChargenPreviewViewportWidget once, but its underlying mount
(CharacterCreationUiMountCoordinator) is explicitly retryable while this
GPU-resource composition pass is not — unlike PaperdollViewportWidget,
which IS eager/non-retryable, so the "mirrors Paperdoll" doc claim was
false. Retrofitting cross-frame retry here would mean restructuring this
composition's one-shot contract for every private viewport (paperdoll,
creature appraisal) and FrameRootComposition's fixed frame-group array —
out of this round's blast radius. Corrected the doc and made the failure
loud (a diagnostic log) instead of silent.
Dispose leak (F9): ChargenPreviewController.Dispose left the preview
WorldEntity referenced by the leased renderer until the renderer's own,
later disposal. Releases it on its own teardown now.
Test-quality items (F10, F11, F13): pinned the spin arrow widths
(47px, both arrows) the 174 zone boundary is derived from, plus a
controller test for the previously-uncovered select zone. Measured the
shade scrollbar's authored orientation instead of assuming it — it is
VERTICAL (33x85) — which is a real production bug: UiScrollbar only routed
scalar-mode mouse events when Horizontal was true, so the shade control
never fired in production. Added OnVerticalScalarEvent/DrawVerticalScalar
mirroring the existing horizontal scalar path. Converted
ChargenPreviewControllerTests from silent-pass [Fact] to the shared
InstalledDatFactAttribute skip-reporting pattern.
Adjudication (F12): AD-101's retirement leaves TryBeginFinish's four local
refusals (NoName/AttributeCreditsUnspent/AlreadyPending/RosterFull) with no
heritage/gender gate — currently latent since Finish stays hard-disabled
this round. Amended the campaign plan's CC5 slice scope to require BOTH a
heritage/gender refusal AND a real RandomizeCharacter port before the
connected user gate opens Finish; noted the interaction on AP-214's own
register row. No CC5 implementation in this commit.
Gates: dotnet build -c Release green across the full solution. App suite
(Release, ACDREAM_PROBE_LIVE_MOUNT=1) 5223/3 skips, Runtime suite
1713/0 — both clean across repeated runs. A full-solution run surfaced
three pre-existing, previously-documented flakes unrelated to this change
(Streaming.LandblockBuildFactoryTests/LandblockPresentationPipelineTests
#402, Core.Net.Tests.NakEmissionTests loss soak) — each confirmed passing
in isolation, consistent with their known full-suite-parallelism-timing
history; none touch any file this commit changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
34c6fceab0
commit
d2a71152d2
14 changed files with 571 additions and 54 deletions
|
|
@ -257,10 +257,19 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
if (_spins.TryGetValue(Part.Mouth, out UiButton? mouthSpin))
|
||||
mouthSpin.Visible = !clothesHidden;
|
||||
_eyesArrowsDisabled = clothesHidden;
|
||||
if (clothesHidden && _currentChoice == Choice.Clothes)
|
||||
if (clothesHidden)
|
||||
{
|
||||
// Update forces SetChoice(ECG_CHOICE_FACE) when Clothes becomes
|
||||
// unreachable so the page never gets stuck showing a hidden tab.
|
||||
// Fix round F3: retail's Gearknight branch
|
||||
// (@0x0047eac6/0x0047eacf) and Olthoi/OlthoiAcid branch
|
||||
// (@0x0047ee32/0x0047ee3b) both call SetChoice(ECG_CHOICE_FACE)
|
||||
// + SetSelection(ECG_PARTS_HAIR) UNCONDITIONALLY — every single
|
||||
// time Update runs while the heritage hides Clothes, not only
|
||||
// when the Clothes tab happened to be showing. A conditional
|
||||
// gate here (checking _currentChoice == Choice.Clothes) missed
|
||||
// the case where _currentPart was Nose or Mouth — both ALSO
|
||||
// hidden by this same branch — while _currentChoice was still
|
||||
// Face: acdream would leave the hidden Nose/Mouth part driving
|
||||
// the shade control; retail always snaps back to Hair.
|
||||
_currentChoice = Choice.Face;
|
||||
_currentPart = Part.Hair;
|
||||
}
|
||||
|
|
@ -334,10 +343,87 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
{
|
||||
if (_disposed)
|
||||
return;
|
||||
NormalizeChoiceOnSelect(part);
|
||||
_currentPart = part;
|
||||
RefreshColorAndShadeControlsFromLatestSnapshot();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Fix round F1: ports retail's spin BODY-click normalize-and-write-back
|
||||
/// — <c>gmCGAppearancePage::ListenToElementMessage</c> cases <c>0xa5</c>-
|
||||
/// <c>0xa9</c> (hair/eyes/nose/mouth/skin, <c>@0x0047f04b-0x0047f1bf</c>)
|
||||
/// and <c>0xab</c>-<c>0xae</c> (headgear/shirt/trousers/footwear,
|
||||
/// <c>@0x0047f212-0x0047f3ac</c>) each re-clamp the part's current index
|
||||
/// into <c>[0, count)</c> BEFORE selecting it as current, not just read
|
||||
/// it. Retail's rule (Hair's case 0xa5 is representative,
|
||||
/// <c>@0x0047f051-0x0047f081</c> plus the shared tail at
|
||||
/// <c>label_47f065</c>/<c>label_47f6d9</c>): <c>cur >= count -> 0</c>;
|
||||
/// <c>cur < 0 -> count-1</c>. Headgear's own case (0xab,
|
||||
/// <c>@0x0047f218-0x0047f23e</c>) excludes its <c>0xffffffff</c> Unset
|
||||
/// sentinel from the "cur < 0" branch
|
||||
/// (<c>iCurrentChoice < 0 && iCurrentChoice != 0xffffffff</c>),
|
||||
/// so an Unset headgear survives a body click untouched; every other
|
||||
/// indexed spin has no such exclusion, so an Unset (AP-214 honest-blank)
|
||||
/// style wraps to <c>count-1</c> on the FIRST body click — the same
|
||||
/// count-1 wrap <see cref="CycleIndex"/>'s own decrement-from-Unset fix
|
||||
/// (F1's sibling finding) applies. Skin (case 0xa9,
|
||||
/// <c>@0x0047f1bf-0x0047f1fb</c>) normalizes its local cache too but
|
||||
/// never writes back (no <c>CharGenState</c> field for Skin — acdream:
|
||||
/// no <see cref="StyleSlotFor"/> case), matching this method's no-op
|
||||
/// early return for it. In acdream there is no separate UI-local cache
|
||||
/// to desync from the persisted index (unlike retail's <c>m_tChoices</c>)
|
||||
/// — <see cref="RuntimeCharacterCreationState.TrySetAppearanceIndex"/>
|
||||
/// already rejects any out-of-range write and
|
||||
/// <c>ConstrainAppearanceByGenderLocked</c> already clamps on every
|
||||
/// gender change — so the ONLY reachable out-of-range case here is
|
||||
/// Unset itself; the >=count branch is kept for completeness/fidelity
|
||||
/// with retail's own defensive shape, not because acdream can hit it.
|
||||
/// </summary>
|
||||
private void NormalizeChoiceOnSelect(Part part)
|
||||
{
|
||||
ChargenAppearanceSlot? slot = StyleSlotFor(part);
|
||||
if (slot is null)
|
||||
return; // Skin: retail normalizes locally but never writes back.
|
||||
|
||||
IRuntimeCharacterCreationView? view = _bindings.View();
|
||||
if (view is null)
|
||||
return;
|
||||
RuntimeCharacterCreationSnapshot snapshot = view.Snapshot;
|
||||
if (!TryGetGender(view, snapshot, out ChargenGenderOptions? gender))
|
||||
return;
|
||||
|
||||
int count = StyleCount(part, gender);
|
||||
if (count <= 0)
|
||||
return;
|
||||
uint current = StyleCurrent(part, snapshot.Appearance);
|
||||
|
||||
uint normalized;
|
||||
if (part == Part.Headgear)
|
||||
{
|
||||
// 0x0047f218/0x0047f226: cur >= count -> Unset; Unset itself
|
||||
// (cur < 0 as signed int32) is explicitly excluded from the
|
||||
// "cur < 0 -> count-1" branch, so it stays Unset.
|
||||
if (current != Unset && current >= (uint)count)
|
||||
normalized = Unset;
|
||||
else
|
||||
return;
|
||||
}
|
||||
else
|
||||
{
|
||||
// 0x0047f04b family: cur >= count -> 0; cur < 0 -> count-1.
|
||||
// Unset (0xFFFFFFFF) reads as -1 in retail's signed int32 store,
|
||||
// so it takes the "cur < 0" branch same as any other negative.
|
||||
if (current != Unset && current >= (uint)count)
|
||||
normalized = 0u;
|
||||
else if (current == Unset)
|
||||
normalized = (uint)(count - 1);
|
||||
else
|
||||
return;
|
||||
}
|
||||
|
||||
_bindings.SetAppearanceIndex?.Invoke(slot.Value, normalized);
|
||||
}
|
||||
|
||||
private void CycleStyle(Part part, int delta)
|
||||
{
|
||||
if (_disposed)
|
||||
|
|
@ -378,14 +464,29 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
/// <paramref name="count"/>+1 positions (every real index, plus
|
||||
/// <see cref="Unset"/> — decrementing from index 0 lands on Unset,
|
||||
/// incrementing from Unset lands on index 0, matching
|
||||
/// <c>ListenToElementMessage</c>'s cases <c>6</c> exactly). Every other
|
||||
/// style spin has no decomp-observable Unset-cycling case (retail always
|
||||
/// has a real 0-based index by the time the user can click — see
|
||||
/// AP-214's <c>RandomizeCharacter</c>-at-open finding, which acdream
|
||||
/// does not port this round) — an Unset start there is an edge case
|
||||
/// retail itself never reaches, so the first click either direction just
|
||||
/// starts cycling from index 0 rather than reconstructing an unfounded
|
||||
/// wrap direction.
|
||||
/// <c>ListenToElementMessage</c>'s cases <c>6</c> exactly).
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Fix round F1:</b> every OTHER style spin ALSO has a decomp-
|
||||
/// observable Unset-cycling case — it lives in the same switch the
|
||||
/// headgear ring was ported from, at the shared decrement tail
|
||||
/// (<c>label_47f065</c>/<c>label_47f6d9</c>, reached from Hair's
|
||||
/// decrement case <c>@0x0047f465-0x0047f486</c> and, inlined per-part,
|
||||
/// from Eyes/Nose/Mouth/Shirt/Trousers/Footwear's own decrement cases
|
||||
/// <c>@0x0047f491-0x0047f65c</c>): decrementing FROM Unset
|
||||
/// (<c>cur=-1</c> as signed int32) computes <c>new = cur - 1 = -2</c>,
|
||||
/// which is <c>< 0</c>, so it wraps to <c>count - 1</c> — the SAME
|
||||
/// "wrap to the last index" shape headgear's own ring uses, just without
|
||||
/// headgear's extra Unset ring position. Incrementing FROM Unset
|
||||
/// computes <c>new = -1 + 1 = 0</c>, which is already in
|
||||
/// <c>[0, count)</c>, so it lands on style 0 — this half was already
|
||||
/// correct. The prior doc here claimed "no decomp-observable
|
||||
/// Unset-cycling case" and picked index 0 for BOTH directions; the
|
||||
/// decomp refutes that for decrement. This matters in practice: AP-214's
|
||||
/// honest-blank open leaves every non-headgear index Unset, so the
|
||||
/// FIRST left-arrow click a user makes on this page hits this exact
|
||||
/// path.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
internal static uint CycleIndex(uint current, int delta, int count, bool allowUnset)
|
||||
{
|
||||
|
|
@ -401,7 +502,22 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
}
|
||||
|
||||
if (current == Unset)
|
||||
return 0u;
|
||||
{
|
||||
// Retail's per-part decrement/increment cases each recompute
|
||||
// `new = cur + delta` on the RAW signed int32 (Unset = -1) and
|
||||
// apply a SINGLE-STEP clamp (not a full modulo): new < 0 wraps
|
||||
// to count-1, new >= count wraps to 0. Since every real caller
|
||||
// only ever passes delta = -1/+1 here, evaluating that one-step
|
||||
// clamp directly (rather than routing Unset through the general
|
||||
// Mod() below, which assumes a valid starting index) reproduces
|
||||
// retail exactly for both directions.
|
||||
int fromUnset = -1 + delta;
|
||||
if (fromUnset < 0)
|
||||
return (uint)(count - 1);
|
||||
if (fromUnset >= count)
|
||||
return 0u;
|
||||
return (uint)fromUnset;
|
||||
}
|
||||
return (uint)Mod((int)current + delta, count);
|
||||
}
|
||||
|
||||
|
|
@ -455,6 +571,23 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
IRuntimeCharacterCreationView view,
|
||||
RuntimeCharacterCreationSnapshot snapshot)
|
||||
{
|
||||
// Fix round F2 item 2: gmCGAppearancePage::SetSelection
|
||||
// @0x0047e260 resets the PREVIOUS current-part spin to state 1
|
||||
// (@0x0047e306, this->m_pCurSelection->vtable->SetState(1)) and sets
|
||||
// the NEW one to state 6 (@0x0047e837,
|
||||
// this->m_pCurSelection->vtable->SetState(6)) — a literal highlight
|
||||
// toggle. UiButtonStateMachine.Normal/Highlight are already retail's
|
||||
// own numeric ids 1/6 (see that class); IUiDatStateful.TrySetRetailState
|
||||
// is the established seam for pushing a raw retail state id
|
||||
// (CharacterCreationUiController.SetMasterPageState's own pattern).
|
||||
foreach ((Part spinPart, UiButton spin) in _spins)
|
||||
{
|
||||
spin.TrySetRetailState(
|
||||
spinPart == _currentPart
|
||||
? UiButtonStateMachine.Highlight
|
||||
: UiButtonStateMachine.Normal);
|
||||
}
|
||||
|
||||
ChargenAppearanceSlot? colorSlot = ColorSlotFor(_currentPart);
|
||||
uint currentColor = colorSlot is null ? Unset : ColorCurrent(_currentPart, snapshot.Appearance);
|
||||
for (int i = 0; i < _swatches.Length; i++)
|
||||
|
|
@ -466,7 +599,12 @@ internal sealed class CharacterCreationAppearancePage : IDisposable
|
|||
ChargenShadeSlot? shadeSlot = ShadeSlotFor(_currentPart);
|
||||
if (_shadeScroll is null)
|
||||
return;
|
||||
_shadeScroll.Enabled = shadeSlot is not null;
|
||||
// Fix round F2 item 3: gmCGAppearancePage::SetSelection HIDES the
|
||||
// shade scrollbar for Eyes (@0x0047e862, SetVisible(0) — Eyes has no
|
||||
// shade case in SetShade at all) and shows it otherwise
|
||||
// (@0x0047e878, SetVisible(1)) — retail never DISABLES it, it
|
||||
// removes it from the layout entirely.
|
||||
_shadeScroll.Visible = shadeSlot is not null;
|
||||
if (shadeSlot is { } slot)
|
||||
{
|
||||
double shade = ShadeCurrent(slot, snapshot.Appearance);
|
||||
|
|
|
|||
|
|
@ -50,9 +50,11 @@ public sealed record CharacterCreationRuntimeBindings(
|
|||
/// through retail <c>gmCharGenMainUI</c>'s authored retained layout — the
|
||||
/// mount + master shell (progress bar, tab strip, Back/Next/Finish/Help/
|
||||
/// Exit/Random nav) plus the Heritage/Profession/Skills/Town pages this
|
||||
/// slice builds. The Appearance (<c>0x100003d4</c>) and Summary
|
||||
/// (<c>0x100003d6</c>) page roots are mounted but content-inert — CC6/CC5
|
||||
/// fill them (register TS-82).
|
||||
/// slice builds. Fix round F6: the Appearance (<c>0x100003d4</c>) page root
|
||||
/// is fully LIVE as of CC6b-MOUNT (<see cref="CharacterCreationAppearancePage"/>);
|
||||
/// only the Summary (<c>0x100003d6</c>) page root remains mounted but
|
||||
/// content-inert — CC5 fills it (register TS-82, narrowed to Summary-only
|
||||
/// at CC6b-MOUNT).
|
||||
///
|
||||
/// <para>
|
||||
/// Decomp anchors: root construction + child resolution
|
||||
|
|
@ -639,10 +641,14 @@ internal sealed class CharacterCreationUiController : IDisposable
|
|||
break;
|
||||
}
|
||||
|
||||
// Random (0x100003cb): retail refuses on Skills (no
|
||||
// RandomizeSkills primitive ported — AP-212) and on Summary
|
||||
// (MakeRandomizeWarningDialog is CC5's); Appearance is this round's
|
||||
// placeholder.
|
||||
// Random (0x100003cb): fix round F5 — retail's DoRandom @0x004e7d70
|
||||
// case 3 fully ENABLES Random on Appearance (RandomizeClothing when
|
||||
// m_eCurType == ECG_CHOICE_CLOTHES, else RandomizeAppearance); this
|
||||
// is NOT a placeholder gap the way the old comment claimed. The
|
||||
// disable here rests on the SAME unported-primitive gap AP-212
|
||||
// tracks for Skills (no RandomizeSkills) and Summary (no
|
||||
// RandomizeCharacter) — RandomizeAppearance/RandomizeClothing are
|
||||
// two more of AP-212's six named-but-unported primitives.
|
||||
_random.Enabled = _currentPage
|
||||
is not (Page.Skills or Page.Appearance or Page.Summary);
|
||||
// Finish stays ghosted regardless of page — Summary is a
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue