fix(chargen): Campaign CC CC6b-PRE review fix round — F1-F7 + F11 concession rewrite
F1 (BLOCKING, doc-only) — the idle-by-default rationale rested on an unsound "uninitialized C++ member defaults to 0" argument (heap operator-new memory is indeterminate, not zero). Verified and replaced with the real evidence: gmCGAppearancePage::InitializePage @0x0047FDD0 writes an EXPLICIT this->m_bZoomedIn = 0; at 0x004802C3, immediately after that same function points the camera at the zoomed-IN per-heritage eye (0x00480286-0x0048029E). Fixed in all three places: the register's TS-83 retirement clause, ChargenPreviewAnimator's class doc, ChargenPreviewZoomController.IsZoomedIn's doc. Recorded the retail quirk this implies: the character starts framed close-up while not-zoomed-in, so the first Zoom In click (once mounted) tweens close-eye->close-eye (visually null) while still freezing the animation — the port reproduces this faithfully. F2 — ChargenPreviewZoomController and ChargenPreviewAnimator kept independent _zoomedIn bools synced only via a nullable animator parameter, risking desync. Retail's m_bZoomedIn is a single field gating both camera and animation, so the fix makes the animator the sole state owner: ChargenPreviewZoomController now takes its ChargenPreviewAnimator as a required constructor dependency, IsZoomedIn reads straight through to it, and ZoomIn/ZoomOut no longer take a parameter at all — there is no second bool left to disagree. F3 — documented the DoRotation counter-clockwise branch's x87-stack decompiler artifact (BN renders x87_r7_1 = x87_r6_3 at 0x0047CAEB, which would store delta-degrees instead of the timestamp for CCW only); the port already stores "now" in both branches, cited against feedback_bn_decomp_field_names.md. F4 — ChargenPreviewAnimator.ApplyIdleFrame now double-buffers two List<MeshRef> instead of allocating fresh every 30fps tick. F5 — filed docs/ISSUES.md #402 tracking the RetailAnimationCyclePlayback / LiveEntityAnimationPresenter duplication as an owned post-CC follow-up, referenced from the new type's own doc. F6 — reworded the ChargenPreviewEntityBuilder.TryBuild "byte-identical" claim to result-identical (TryBuildAnimated now also resolves the idle DID and loads the idle Animation before the wrapper discards them). F7 — added the missing clockwise >360 clamp test (readable decomp polarity, unlike F3's CCW artifact). ALSO — rewrote the CC6b ledger row's m_alternateSetupID MUST-COVER note per the reviewer's F11 concession: all five write sites belong to gmBarberUI (the post-creation barber shop), not gmCGAppearancePage, which has no option-checkbox-equivalent field at all. Added the enclosing-function citations and an explicit directive that CC6b-mount must NOT build a crown/no-flame checkbox on the Appearance page. Tests: ChargenPreviewRotationControllerTests +1 (10 total), ChargenPreviewZoomControllerTests +2 and every case rewritten for the required-animator constructor (9 total). Core.Tests 4786/1 skip (unchanged), Content.Tests 147/0, App.Tests 5152/6 skips (+3) — zero failures in isolation, full solution Release build green. Two pre-existing flakes observed across repeated full-solution runs, neither caused by this round and neither reproducing standalone: Core.Net.Tests' NakEmissionTests loss soak, and Content.Tests' DecodedTextureCacheTests concurrency race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
8dfee1118f
commit
1ba22a01a8
10 changed files with 195 additions and 50 deletions
|
|
@ -24,6 +24,39 @@ 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.
|
||||
|
||||
## #402 — Consolidate RetailAnimationCyclePlayback into LiveEntityAnimationPresenter's legacy branch
|
||||
|
||||
**Status:** OPEN (post-CC consolidation follow-up)
|
||||
**Severity:** LOW
|
||||
**Filed:** 2026-08-15 (Campaign CC slice CC6b-PRE review fix round, F5)
|
||||
**Component:** `src/AcDream.Core/Physics/RetailAnimationCyclePlayback.cs`,
|
||||
`src/AcDream.App/Rendering/LiveEntityAnimationPresenter.cs`
|
||||
|
||||
`RetailAnimationCyclePlayback` (advance-with-wrap + lerp/slerp) is a Core,
|
||||
pure, unit-tested extraction of the SAME algorithm
|
||||
`LiveEntityAnimationPresenter.Present`'s legacy (no-`AnimationSequencer`)
|
||||
branch already carries inline for NPC idle cycles
|
||||
(`CurrFrame += legacyAdvanceSeconds * Framerate` with the same modulo wrap,
|
||||
plus its own private `TryResolvePartFrame` doing the same frame-bracket
|
||||
lerp/slerp). The chargen preview (`ChargenPreviewAnimator`) consumes the
|
||||
new shared type; the two implementations were deliberately left
|
||||
un-consolidated at CC6b-PRE — `LiveEntityAnimationPresenter` is live,
|
||||
heavily-tested, in-flight production entity-rendering code with zero
|
||||
relation to the preview-only feature that motivated the extraction, so
|
||||
touching it was judged out of that slice's blast radius.
|
||||
|
||||
That decision has no tracked owner. Someone should, in a dedicated pass
|
||||
after Campaign CC closes: redirect `LiveEntityAnimationPresenter`'s inline
|
||||
copy through `RetailAnimationCyclePlayback` (a behavior-preserving
|
||||
mechanical swap — same formulas, same order of operations) and delete the
|
||||
duplicate. Verify byte-identical output first (a differential test against
|
||||
the pre-change behavior over a representative NPC idle set) before landing.
|
||||
|
||||
**Acceptance:** one call site for the advance-with-wrap + lerp/slerp
|
||||
algorithm; `LiveEntityAnimationPresenter`'s legacy branch calls
|
||||
`RetailAnimationCyclePlayback` instead of reimplementing it; no behavior
|
||||
change to any currently-animated NPC.
|
||||
|
||||
## #401 — RetailUi should default ON (opt-out), not per-path forced
|
||||
|
||||
**Status:** OPEN (product-default decision)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue