fix(chargen): Campaign CC CC5 re-review residuals R1-R5 + nits — REVIEW-CLOSED
The narrow re-review of fix commits 0c8e1e7d+2d4168f9 found every code fix
oracle-verified but returned NOT CLOSED on five test/doc residuals plus
three nits and a follow-up filing. All fixed:
- R1: the claimed "external-change->user-commit->SetName" regression test
for F1 (the deleted _suppressNextFieldEvent latch) never existed — the
Runtime-layer randomize test doesn't touch the page. Added
CharacterCreationUiControllerTests.SummaryNameField_RealCommitAfter
ExternalRefreshWhileUnfocused_StillReachesSetName: drives Refresh with a
revision bump + changed snapshot.Name while the field is unfocused (the
programmatic SetText path that used to arm the latch), THEN performs a
real user commit (field.SetText + field.Submit(), the actual event path),
asserting SetName receives the player's typed text.
- R2: RetailSkillFormula.CalculateChargenScore and ChargenSkillScoreResolver
had zero direct coverage (the F12(d) test substitutes skillId*10). Added
a Untrained/Trained(+5)/Specialized(+10) theory, the divisor-zero skip
path, and a six-way AttributeId theory (Str=1..Self=6) to
RetailSkillFormulaTests.cs.
- R3: RetailSkillFormula.cs's doc comment claimed "no retail-authored skill
sets MinLevel above Untrained=1" without ever reading the field — ACE's
own SkillBase.cs hedges the same field "// 1-2?". MEASURED (not assumed)
against the installed EoR dat's global SkillTable
(CharacterCreationLiveDatTests.SkillTable_MinLevelDistribution_
NeverExceedsTrained): 23 skills at MinLevel 1, 15 at MinLevel 2, zero
above 2, of 38 priced skills. ACE's hedge was right; the doc comment now
states the measured fact and leans on the structural argument (the gate
holds for Trained/Specialized under any MinLevel in {1,2}) as load-
bearing, not the unverified data claim.
- R4: filed AP-228 — the Summary/Skills skill-row KEY sources from
ItemAppraisalTextFormatter.SkillName's hardcoded English switch, where
retail's own key is DAT-sourced (SkillBase->_name via %hs,
0x0047b90f-0x0047b915) — same divergence class as AP-226 filed the same
round, reversed polarity, also present at CC4's Skills page. Softened
AP-224's "ported exactly, not simplified" claim: it only ever covered the
row's VALUE/template, never its KEY.
- R5: this commit corrects 0c8e1e7d's gate claim. "Release build zero
warnings" was false: a clean `dotnet build -c Release -t:Rebuild` shows
25 pre-existing warnings (18 in tests/AcDream.Core.Tests, 7 in
tests/AcDream.App.Tests — Composition/HostInputCameraCompositionTests.cs,
Composition/WorldRenderCompositionTests.cs,
UI/Layout/OptionsPanelLiveMountProbeTests.cs), none in any file this
campaign or its residual round touched. History is not amended; this is
the correction.
Nits: the ChargenPreviewController ctor doc now also cites
gmCGSummaryPage::Update @0x0047baa0 (the per-heritage re-derive site — 0xc
Olthoi/0xd OlthoiAcid/else — not just the one-shot InitializePage seed) as
the stronger justification for why Rebuild re-derives the zoomed-out eye
per heritage on every change. RuntimeCharacterCreationState's F2 comment
("Finish becoming a permanent no-op") reworded: the same unconditional
_verificationPending = false assignment ran pre-fix too, so Finish was
never blocked — only the response FEEDBACK vanished (no dialog, no created
character, nothing), not the request itself. Filed #404 for
ChargenSkillScoreResolver's own independent SkillTable read alongside
ChargenTableReader's (cleanup follow-up, out of this round's scope).
Ledger: CC5 flipped REVIEW-CLOSED in the campaign plan (dual-lens
architectural PASS-with-items / retail-fidelity FAIL -> F1-F14 fix round
0c8e1e7d -> narrow re-review: all code oracle-verified, residuals R1-R5
test/doc -> this commit; re-reviewer pre-authorized lead diff-check close).
This commit's own sha is recorded by a follow-up ledger-only commit,
matching 2d4168f9's own pattern.
Gates: Release build 0 errors (25 pre-existing warnings, unrelated to this
round — see R5 above); App suite 5257/3 skips (was 5242/3), 0 failed;
Runtime suite 1726/0 (unchanged); the three new/measured tests (R1, R2's
ten cases, R3) all pass individually.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
2d4168f926
commit
356545c530
9 changed files with 277 additions and 13 deletions
|
|
@ -1,4 +1,5 @@
|
|||
using AcDream.App.Net;
|
||||
using AcDream.Core.CharGen;
|
||||
using DatReaderWriter.DBObjs;
|
||||
using DatReaderWriter.Enums;
|
||||
using DatReaderWriter.Types;
|
||||
|
|
@ -109,4 +110,104 @@ public sealed class RetailSkillFormulaTests
|
|||
Attribute2Multiplier = y,
|
||||
Divisor = unchecked((int)z),
|
||||
};
|
||||
|
||||
// ── CC5 re-review residual round, R2 (2026-08-16): RetailSkillFormula.
|
||||
// ── CalculateChargenScore / ChargenSkillScoreResolver had zero direct
|
||||
// ── coverage — the F12(d) test substitutes `skillId * 10` instead of
|
||||
// ── exercising the real formula. ─────────────────────────────────────
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="RetailSkillFormula.CalculateChargenScore"/>'s level bonus:
|
||||
/// Untrained adds nothing, Trained adds 5, Specialized adds 10, on top
|
||||
/// of the SAME base <see cref="RetailSkillFormula.TryCalculate"/> result
|
||||
/// (formula here: <c>w=0, x=1, y=0, z=1</c> against
|
||||
/// <c>attribute1=5</c> -> base 5).
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData(ChargenSkillAdvancementClass.Untrained, 5u)]
|
||||
[InlineData(ChargenSkillAdvancementClass.Trained, 10u)]
|
||||
[InlineData(ChargenSkillAdvancementClass.Specialized, 15u)]
|
||||
public void CalculateChargenScore_AddsTheLevelBonusOnTopOfTheBaseFormula(
|
||||
ChargenSkillAdvancementClass level,
|
||||
uint expected)
|
||||
{
|
||||
var skillBase = new SkillBase { Formula = Formula(w: 0, x: 1, y: 0, z: 1) };
|
||||
|
||||
uint result = RetailSkillFormula.CalculateChargenScore(skillBase, attribute1: 5u, attribute2: 0u, level);
|
||||
|
||||
Assert.Equal(expected, result);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The divisor-zero skip path: <see cref="RetailSkillFormula.TryCalculate"/>'s
|
||||
/// own failure gate short-circuits <see cref="RetailSkillFormula.CalculateChargenScore"/>
|
||||
/// to a flat 0 BEFORE the level bonus is ever added — even for
|
||||
/// Specialized, which would otherwise add 10.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void CalculateChargenScore_ZeroDivisor_ReturnsZero_RegardlessOfLevel()
|
||||
{
|
||||
var skillBase = new SkillBase { Formula = Formula(w: 7, x: 1, y: 1, z: 0) };
|
||||
|
||||
Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
|
||||
skillBase, 10u, 20u, ChargenSkillAdvancementClass.Untrained));
|
||||
Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
|
||||
skillBase, 10u, 20u, ChargenSkillAdvancementClass.Trained));
|
||||
Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
|
||||
skillBase, 10u, 20u, ChargenSkillAdvancementClass.Specialized));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="ChargenSkillScoreResolver.Resolve"/>'s six-way
|
||||
/// <c>ResolveAttribute</c> switch — one case per
|
||||
/// <see cref="AttributeId"/> (Strength=1..Self=6) — resolving through a
|
||||
/// formula that reads ONLY <c>Attribute1</c> (<c>x=1, y=0</c>) so the
|
||||
/// result unambiguously reports which of the six
|
||||
/// <see cref="ChargenAttributeValues"/> fields the switch actually read.
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData(AttributeId.Strength)]
|
||||
[InlineData(AttributeId.Endurance)]
|
||||
[InlineData(AttributeId.Coordination)]
|
||||
[InlineData(AttributeId.Quickness)]
|
||||
[InlineData(AttributeId.Focus)]
|
||||
[InlineData(AttributeId.Self)]
|
||||
public void ChargenSkillScoreResolver_ResolvesEachAttributeIdThroughTheSixWaySwitch(
|
||||
AttributeId attributeId)
|
||||
{
|
||||
const uint skillId = 0x10u;
|
||||
var skillTable = new SkillTable();
|
||||
skillTable.Skills.Add((SkillId)skillId, new SkillBase
|
||||
{
|
||||
Formula = new SkillFormula
|
||||
{
|
||||
AdditiveBonus = 0,
|
||||
Attribute1Multiplier = 1,
|
||||
Attribute2Multiplier = 0,
|
||||
Divisor = 1,
|
||||
Attribute1 = attributeId,
|
||||
// Attribute2 deliberately left at its zero default (Strength) —
|
||||
// Attribute2Multiplier=0 means whatever it reads contributes
|
||||
// nothing, so it cannot mask a wrong Attribute1 case.
|
||||
},
|
||||
});
|
||||
var resolver = new ChargenSkillScoreResolver(skillTable);
|
||||
ChargenAttributeValues attributes = AttributeValuesWith(attributeId, 42);
|
||||
|
||||
uint result = resolver.Resolve(skillId, attributes, ChargenSkillAdvancementClass.Untrained);
|
||||
|
||||
Assert.Equal(42u, result);
|
||||
}
|
||||
|
||||
private static ChargenAttributeValues AttributeValuesWith(AttributeId attributeId, int value) =>
|
||||
attributeId switch
|
||||
{
|
||||
AttributeId.Strength => new ChargenAttributeValues(value, 0, 0, 0, 0, 0),
|
||||
AttributeId.Endurance => new ChargenAttributeValues(0, value, 0, 0, 0, 0),
|
||||
AttributeId.Coordination => new ChargenAttributeValues(0, 0, value, 0, 0, 0),
|
||||
AttributeId.Quickness => new ChargenAttributeValues(0, 0, 0, value, 0, 0),
|
||||
AttributeId.Focus => new ChargenAttributeValues(0, 0, 0, 0, value, 0),
|
||||
AttributeId.Self => new ChargenAttributeValues(0, 0, 0, 0, 0, value),
|
||||
_ => default,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -572,6 +572,51 @@ public sealed class CharacterCreationLiveDatTests
|
|||
Assert.IsType<UiText>(UiElement.FindDescendant(pairRow, 0x100002FDu));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// CC5 re-review residual round, R3 (2026-08-16): MEASURES the
|
||||
/// installed global SkillTable's (portal.dat <c>0x0E000004</c>)
|
||||
/// <c>MinLevel</c> distribution instead of inferring it — the C4
|
||||
/// closeout's "observe, don't infer" lesson
|
||||
/// (<c>docs/research/2026-08-05-c4-closeout-handoff.md</c>).
|
||||
/// <see cref="AcDream.App.Net.RetailSkillFormula.CalculateChargenScore"/>'s
|
||||
/// own doc comment used to claim "no retail-authored skill sets
|
||||
/// MinLevel above Untrained=1" without ever reading the field; ACE's own
|
||||
/// <c>SkillBase.cs</c> annotates the same field <c>// 1-2?</c> (a hedge
|
||||
/// that observed 2s exist). This is that measurement — see the method's
|
||||
/// own real-DAT finding recorded in <c>RetailSkillFormula.cs</c>'s doc
|
||||
/// comment, corrected from this test's result.
|
||||
/// </summary>
|
||||
[InstalledDatFact]
|
||||
public void SkillTable_MinLevelDistribution_NeverExceedsTrained()
|
||||
{
|
||||
using var dats = new DatCollection(DatDirectory, DatAccessType.Read);
|
||||
DatReaderWriter.DBObjs.SkillTable? skillTable =
|
||||
dats.Get<DatReaderWriter.DBObjs.SkillTable>(0x0E000004u);
|
||||
Assert.NotNull(skillTable);
|
||||
|
||||
var byMinLevel = skillTable!.Skills.Values
|
||||
.GroupBy(skill => skill.MinLevel)
|
||||
.OrderBy(g => g.Key)
|
||||
.ToDictionary(g => g.Key, g => g.Count());
|
||||
Console.WriteLine(
|
||||
"[CC5-R3-DAT] SkillTable MinLevel distribution (level=count): "
|
||||
+ string.Join(", ", byMinLevel.Select(p => $"{p.Key}={p.Value}"))
|
||||
+ $" (total skills: {skillTable.Skills.Count})");
|
||||
|
||||
// RetailSkillFormula.CalculateChargenScore's own gate argument ("the
|
||||
// decomp's own `if (edi_1 >= MinLevel)` gate passes for both callers,
|
||||
// Trained=2 and Specialized=3") only holds while every skill's
|
||||
// MinLevel stays at or below Trained(2) — pin that as a real
|
||||
// assertion instead of leaving it as prose, so a future DAT
|
||||
// revision that adds a MinLevel=3+ skill fails HERE, not silently
|
||||
// under-credits that skill's chargen score.
|
||||
Assert.True(
|
||||
byMinLevel.Keys.All(minLevel => minLevel <= 2),
|
||||
"A skill's MinLevel exceeded 2 (Trained) in the installed DAT — "
|
||||
+ "RetailSkillFormula.CalculateChargenScore's Trained/Specialized "
|
||||
+ "gate argument needs re-verification for this skill.");
|
||||
}
|
||||
|
||||
private static void AssertButton(ImportedLayout layout, uint elementId) =>
|
||||
Assert.IsType<UiButton>(layout.FindElement(elementId));
|
||||
|
||||
|
|
|
|||
|
|
@ -1055,6 +1055,51 @@ public sealed class CharacterCreationUiControllerTests
|
|||
Assert.False(field.CharacterFilter!('$'));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// CC5 re-review residual round, R1 (2026-08-16): pins the F1 fix (the
|
||||
/// deleted <c>_suppressNextFieldEvent</c> latch) against reintroduction.
|
||||
/// The pre-fix bug needed BOTH halves to reproduce: (1) an EXTERNAL
|
||||
/// change to <c>snapshot.Name</c> lands while the field is unfocused —
|
||||
/// <c>CharacterCreationSummaryPage.Refresh</c>'s own field-sync block
|
||||
/// (the F1 fix site) calls <c>field.SetText(...)</c> programmatically,
|
||||
/// which armed the old latch — then (2) the PLAYER's own REAL commit
|
||||
/// (<c>SetText</c> + <c>Submit</c>, the actual event path a keystroke +
|
||||
/// Enter/blur drives — never a direct <c>CommitNameFromField</c> call)
|
||||
/// arrives afterward. Pre-fix, that real commit hit the still-armed
|
||||
/// latch and was silently dropped; <see cref="SummaryNameField_Submit_CommitsTheName"/>
|
||||
/// alone never exercised this because it never drives step (1) first —
|
||||
/// the review's own finding was that the claimed regression coverage
|
||||
/// didn't actually touch the page.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SummaryNameField_RealCommitAfterExternalRefreshWhileUnfocused_StillReachesSetName()
|
||||
{
|
||||
using var environment = new EnvironmentHarness();
|
||||
environment.Controller.Open();
|
||||
GoToSummary(environment);
|
||||
UiField field = environment.SummaryNameField();
|
||||
|
||||
// (1) External change while unfocused: something OTHER than this
|
||||
// field bumps the Runtime revision with a changed Name (e.g. the
|
||||
// player edited another page and came back) — Refresh's field-sync
|
||||
// block programmatically overwrites the field via SetText, which
|
||||
// never raises OnFocusLost/OnSubmit.
|
||||
RuntimeCharacterCreationSnapshot snapshot = environment.Runtime.View.Snapshot;
|
||||
environment.Runtime.View.Snapshot = snapshot with
|
||||
{
|
||||
Revision = snapshot.Revision + 1,
|
||||
Name = "Zorak",
|
||||
};
|
||||
environment.Controller.Tick();
|
||||
Assert.Equal("Zorak", field.Text);
|
||||
|
||||
// (2) The player's own real commit afterward.
|
||||
field.SetText("Adventurer");
|
||||
field.Submit();
|
||||
|
||||
Assert.Equal("Adventurer", environment.Runtime.LastSetName);
|
||||
}
|
||||
|
||||
// ── CC5 review fix round F12(d): RebuildListbox content ─────────────
|
||||
|
||||
/// <summary>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue