fix(runtime): Campaign CC slice CC3 review-fix round — F1-F16
Opus dual-lens review of CC3's RuntimeCharacterCreationState passed on retail fidelity but failed the controller integration: the post-create log-straight-in indexed the CACHED wire CharacterList, which ACE never resends after a create (it only appends server-side and replies Ok) — with zero pre-existing characters this throws, with N it can silently enter the WRONG character. The same stale-index problem corrupted every pre-existing character's delete slot on roster re-sort. Fixes all four blocking findings plus a credit-gate correctness bug (retail warns and lets the user confirm through unspent credits; it does not force a full spend) and eight lower-severity findings from the same review round. F1 (blocking): WorldSession gained a guid-based EnterWorld(uint,string, TimeSpan?) overload sharing EnterWorldCore with the index-based one; ILiveSessionOperations gained a default EnterWorldByGuid method. LiveSessionController factored EnterSelectedCore/the new EnterCreatedCharacterCore through a shared EnterHighlightedCore so the post-create enter sends by the exact guid the 0xF643 Ok reply carried, never by a roster index. F2 (blocking): RuntimeCharacterSelectionState gained a real AppendCreatedCharacter primitive that preserves every existing entry's ActiveIndex (a wire contract — SendDeleteCharacter sends it as the CharacterSet slot) and assigns the new entry's from the pre-create wire roster count, instead of round-tripping the post-create roster through ApplyRoster's name-sort-and-renumber. F3 (blocking): retail's DoFinish(this, arg2) gate is "arg2 != 0 && remainingAtrbCredits > 0" — the ordinary click warns and refuses, but the warning dialog's own confirm re-invokes DoFinish(this, 0), which sends anyway with credits unspent (ACE accepts this). TryBeginFinish/Finish gained a confirmedUnspentCredits parameter; the plan doc's "retail FORCES full spend" line is corrected in the same commit. F4 (blocking): a stale out-of-range template index surviving a heritage switch to a heritage with fewer templates now clears to TemplateUnset, matching ConstrainAllByHeritage's clamp. F5/F9/F10: three register-row/doc citation corrections (AP-207's real FitTemplateToCharacter call sites — a fourth one the original filing also missed; the Slot field's real retail assignment source; AP-209's classID branch table for Olthoi/OlthoiAcid). F6: ApplyCreationResponse no longer publishes from inside the owner lock. F7: two new tests pin BalanceAttributes' persistent donor cursor (successive-overspend advance, Self-to-Strength wrap). F8: ResetSkillLevels' doc corrected to retail's real both-costs->=0 gate. F11: the integration test fixture captures guid-based enter calls and uses two pre-existing characters whose wire order differs from alphabetical order, so the roster assertion actually exercises F2 instead of coinciding with it by accident. F12: filed register row AP-211 for the client-side RosterFull slot-cap refusal (no retail DoFinish-layer counterpart). F13: narrowed Finish's bare catch to InvalidOperationException/SocketException and bound _scope to a local. F15: RandomizeStartAreaLocked leaves the start area unchanged on an empty list instead of forcing -1, matching retail. Runtime 1706/0 (was 1701), Core.Net unchanged at 994/0, full solution Release build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
9a84230c4f
commit
397ccd62cd
9 changed files with 578 additions and 68 deletions
|
|
@ -165,6 +165,33 @@ public sealed class RuntimeCharacterCreationStateTests
|
|||
Assert.Equal(new ChargenAttributeValues(10, 10, 10, 10, 10, 10), state.Snapshot.Attributes);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F4 acceptance gate: <c>CharGenState::ConstrainAllByHeritage @
|
||||
/// 0x005C65CC</c> clamps a stale template index to <c>0xffffffff</c>
|
||||
/// when it no longer fits the newly selected heritage's template list.
|
||||
/// Without this, a high template index chosen against a
|
||||
/// many-templates heritage would survive a switch to a heritage with
|
||||
/// fewer templates and reach the wire via <c>BuildRequestLocked</c>.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void TrySelectHeritage_TemplateOutOfRangeForNewHeritage_ClearsToUnset()
|
||||
{
|
||||
RuntimeCharacterCreationState state = CreateActive();
|
||||
state.TrySelectHeritage(RuntimeCharacterCreationStateFixture.AluvianId);
|
||||
state.TrySelectGender(RuntimeCharacterCreationStateFixture.MaleGenderKey);
|
||||
// Index 1 — valid for Aluvian's two templates (Custom=0, Preset=1).
|
||||
Assert.True(state.TrySelectTemplate(RuntimeCharacterCreationStateFixture.PresetTemplateIndex));
|
||||
Assert.Equal(1u, state.Snapshot.Template);
|
||||
|
||||
// Impoverished has only ONE template (index 0) — index 1 no longer
|
||||
// fits; gender (Male) stays valid for Impoverished too, so the
|
||||
// clamp branch (not the "no gender yet" no-op branch) is the one
|
||||
// under test.
|
||||
Assert.True(state.TrySelectHeritage(RuntimeCharacterCreationStateFixture.ImpoverishedId));
|
||||
|
||||
Assert.Equal(RuntimeCharacterCreationSnapshot.TemplateUnset, state.Snapshot.Template);
|
||||
}
|
||||
|
||||
// ── Attributes ──────────────────────────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
|
|
@ -230,6 +257,95 @@ public sealed class RuntimeCharacterCreationStateTests
|
|||
Assert.Equal(16, state.Snapshot.Attributes.Strength);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F7 acceptance gate: <c>CharGenState::BalanceAttributes @
|
||||
/// 0x005C3DF0</c>'s persistent cursor (ported as the instance field
|
||||
/// <c>_attributeBalanceCursor</c>) advances past whichever attribute
|
||||
/// last absorbed an overspend, so a SECOND overspend in a LATER call
|
||||
/// does not re-drain the SAME donor the first call already emptied.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void TrySetAttribute_SuccessiveOverspends_AbsorbFromDifferentAttributes()
|
||||
{
|
||||
RuntimeCharacterCreationState state = CreateActive();
|
||||
state.TrySelectHeritage(RuntimeCharacterCreationStateFixture.AluvianId);
|
||||
state.TrySelectGender(RuntimeCharacterCreationStateFixture.MaleGenderKey);
|
||||
// Str=16, everyone else at the 10 floor, fully spent (66/66) — the
|
||||
// fixture's only above-floor attribute at the start.
|
||||
state.TrySelectTemplate(RuntimeCharacterCreationStateFixture.PresetTemplateIndex);
|
||||
|
||||
// First overspend: raising Endurance by 1 forces a 1-point
|
||||
// donation. The cursor starts at Strength (the only above-floor
|
||||
// attribute), so Strength donates.
|
||||
Assert.True(state.TrySetAttribute(ChargenAttributeId.Endurance, 11));
|
||||
Assert.Equal(15, state.Snapshot.Attributes.Strength);
|
||||
Assert.Equal(11, state.Snapshot.Attributes.Endurance);
|
||||
|
||||
// Second overspend: raising Coordination by 1 forces another
|
||||
// 1-point donation. If the cursor had reset to Strength, Strength
|
||||
// (still above floor at 15) would donate again — it doesn't: the
|
||||
// cursor advanced past Strength after the first call, so THIS
|
||||
// donation comes from Endurance (the attribute the FIRST call just
|
||||
// raised) instead.
|
||||
Assert.True(state.TrySetAttribute(ChargenAttributeId.Coordination, 11));
|
||||
Assert.Equal(15, state.Snapshot.Attributes.Strength); // untouched this time
|
||||
Assert.Equal(10, state.Snapshot.Attributes.Endurance); // donated
|
||||
Assert.Equal(11, state.Snapshot.Attributes.Coordination);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F7 acceptance gate, the wrap case: when the donor found in one pass
|
||||
/// is the LAST entry in the fixed round-robin order (Self — see
|
||||
/// <c>BalanceOrder</c>'s own doc comment: Strength, Endurance,
|
||||
/// Coordination, Quickness, Focus, Self), the cursor wraps back to the
|
||||
/// FIRST entry (Strength) rather than falling off the end.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void TrySetAttribute_BalanceCursor_WrapsFromSelfBackToStrength()
|
||||
{
|
||||
RuntimeCharacterCreationState state = CreateActive();
|
||||
state.TrySelectHeritage(RuntimeCharacterCreationStateFixture.AluvianId);
|
||||
state.TrySelectGender(RuntimeCharacterCreationStateFixture.MaleGenderKey);
|
||||
// Str=16, everyone else at the 10 floor, fully spent (66/66).
|
||||
state.TrySelectTemplate(RuntimeCharacterCreationStateFixture.PresetTemplateIndex);
|
||||
|
||||
// Lock every attribute except Strength and Self: they stay in the
|
||||
// budget total but are excluded from donation, isolating the wrap
|
||||
// behavior to exactly the two attributes under test.
|
||||
Assert.True(state.TrySetAttributeLock(ChargenAttributeId.Endurance, true));
|
||||
Assert.True(state.TrySetAttributeLock(ChargenAttributeId.Coordination, true));
|
||||
Assert.True(state.TrySetAttributeLock(ChargenAttributeId.Quickness, true));
|
||||
Assert.True(state.TrySetAttributeLock(ChargenAttributeId.Focus, true));
|
||||
|
||||
// Move all 6 spare points from Strength to Self — Strength is the
|
||||
// sole eligible donor (everything else is locked or is the raise
|
||||
// target), so it donates all 6. The cursor lands just past
|
||||
// Strength (index 0 → Endurance).
|
||||
Assert.True(state.TrySetAttribute(ChargenAttributeId.Self, 16));
|
||||
Assert.Equal(10, state.Snapshot.Attributes.Strength);
|
||||
Assert.Equal(16, state.Snapshot.Attributes.Self);
|
||||
|
||||
// Raise Strength by 1: every locked attribute is skipped, so the
|
||||
// search reaches Self (the only remaining eligible donor). Self is
|
||||
// the LAST entry in the round-robin order, so this absorption
|
||||
// wraps the cursor back to Strength (the FIRST entry) afterward.
|
||||
Assert.True(state.TrySetAttribute(ChargenAttributeId.Strength, 11));
|
||||
Assert.Equal(11, state.Snapshot.Attributes.Strength);
|
||||
Assert.Equal(15, state.Snapshot.Attributes.Self);
|
||||
|
||||
// Raise the (locked) Endurance attribute by 1 — locking only
|
||||
// excludes an attribute from AUTOMATIC donation, not from being set
|
||||
// directly. If the cursor wrapped correctly, the donor search
|
||||
// starts at Strength again and Strength (still above floor at 11)
|
||||
// donates FIRST — not Self (also still above floor at 15), which is
|
||||
// what an un-wrapped cursor stuck past Self would have picked
|
||||
// instead.
|
||||
Assert.True(state.TrySetAttribute(ChargenAttributeId.Endurance, 11));
|
||||
Assert.Equal(10, state.Snapshot.Attributes.Strength);
|
||||
Assert.Equal(15, state.Snapshot.Attributes.Self); // unchanged — proves the wrap
|
||||
Assert.Equal(11, state.Snapshot.Attributes.Endurance);
|
||||
}
|
||||
|
||||
// ── Skills ──────────────────────────────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
|
|
@ -342,6 +458,40 @@ public sealed class RuntimeCharacterCreationStateTests
|
|||
Assert.True(refusal.AttributeCreditsUnspent);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F3 acceptance gate: <c>gmCharGenMainUI::DoFinish(this, arg2) @
|
||||
/// 0x004E9170</c>'s credit gate is <c>arg2 != 0 &&
|
||||
/// remainingAtrbCredits > 0</c> — retail does NOT force a full
|
||||
/// spend. The ordinary click warns and refuses
|
||||
/// (<c>arg2 = 1 @ 0x004E9579</c>, tested above); the credit-warning
|
||||
/// dialog's own confirm handler re-invokes <c>DoFinish(this, 0)</c>
|
||||
/// (@0x004E98BB), which skips the check entirely and sends with the
|
||||
/// credits still unspent. <c>confirmedUnspentCredits: true</c> is that
|
||||
/// <c>arg2 == 0</c> case.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void TryBeginFinish_UnspentAttributeCreditsConfirmed_IsAccepted()
|
||||
{
|
||||
RuntimeCharacterCreationState state = CreateActive();
|
||||
state.TrySelectHeritage(RuntimeCharacterCreationStateFixture.AluvianId);
|
||||
state.TrySelectGender(RuntimeCharacterCreationStateFixture.MaleGenderKey);
|
||||
state.TrySelectTemplate(RuntimeCharacterCreationStateFixture.CustomTemplateIndex); // 6 unspent
|
||||
state.TrySetName("Adventurer");
|
||||
Assert.Equal(6, state.Snapshot.RemainingAttributeCredits);
|
||||
|
||||
bool accepted = state.TryBeginFinish(
|
||||
0, 11, out CharacterCreate.Request request, out _,
|
||||
out RuntimeCharacterCreationLocalRefusal refusal,
|
||||
confirmedUnspentCredits: true);
|
||||
|
||||
Assert.True(accepted);
|
||||
Assert.False(refusal.Any);
|
||||
Assert.True(state.Snapshot.VerificationPending);
|
||||
// The wire request carries the credits AS UNSPENT — confirming does
|
||||
// not force-spend them, it only skips the local refusal.
|
||||
Assert.Equal(10u, request.Attributes.Strength);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryBeginFinish_SecondCallWhilePending_IsRefused()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -49,6 +49,13 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
public List<WorldSession> Sessions { get; } = [];
|
||||
public int EnterWorldCount { get; private set; }
|
||||
|
||||
/// <summary>F1/F11: captures every guid-based enter call so the
|
||||
/// post-create log-straight-in can be asserted against the EXACT
|
||||
/// identity sent, instead of a bare counter that cannot tell an
|
||||
/// index-based call from a guid-based one, or a right character
|
||||
/// from a wrong one.</summary>
|
||||
public List<(uint Guid, string AccountName)> EnterWorldByGuidCalls { get; } = [];
|
||||
|
||||
public IPEndPoint ResolveEndpoint(string host, int port) =>
|
||||
new(IPAddress.Loopback, port);
|
||||
|
||||
|
|
@ -63,9 +70,19 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
|
||||
public void StartCharacterSelectionReceive(WorldSession session) { }
|
||||
|
||||
/// <summary>Two pre-existing characters whose WIRE order (array
|
||||
/// position — "Zed" slot 0, "Amy" slot 1) deliberately differs from
|
||||
/// their ALPHABETICAL display order ("Amy" < "Zed") — F2's
|
||||
/// regression gate. A single-character roster cannot distinguish a
|
||||
/// correct wire-index-preserving append from a buggy
|
||||
/// re-sort-and-renumber, because with only one entry the two orders
|
||||
/// coincide.</summary>
|
||||
public CharacterList.Parsed? GetCharacters(WorldSession session) => new(
|
||||
0u,
|
||||
[new CharacterList.Character(0x50000001u, "Existing", 0u)],
|
||||
[
|
||||
new CharacterList.Character(0x50000002u, "Zed", 0u),
|
||||
new CharacterList.Character(0x50000003u, "Amy", 0u),
|
||||
],
|
||||
[],
|
||||
SlotCount: 11,
|
||||
AccountName: "testaccount",
|
||||
|
|
@ -75,6 +92,12 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
public void EnterWorld(WorldSession session, int activeCharacterIndex) =>
|
||||
EnterWorldCount++;
|
||||
|
||||
public void EnterWorldByGuid(
|
||||
WorldSession session,
|
||||
uint characterGuid,
|
||||
string accountName) =>
|
||||
EnterWorldByGuidCalls.Add((characterGuid, accountName));
|
||||
|
||||
public void Tick(WorldSession session) { }
|
||||
|
||||
public void DisposeSession(WorldSession session) { }
|
||||
|
|
@ -195,17 +218,36 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
Assert.Equal("NewChar", host.Created[0].Name);
|
||||
Assert.Empty(host.Failed);
|
||||
|
||||
// The roster report following the Ok reply has BOTH the pre-existing
|
||||
// The roster report following the Ok reply has EVERY pre-existing
|
||||
// character and the newly created one.
|
||||
LiveSessionRosterReport lastReport = host.Rosters[^1];
|
||||
Assert.Contains(lastReport.Entries, e => e.Id == 0x50001234u && e.Name == "NewChar");
|
||||
Assert.Contains(lastReport.Entries, e => e.Id == 0x50000001u);
|
||||
Assert.Contains(lastReport.Entries, e => e.Id == 0x50000002u && e.Name == "Zed");
|
||||
Assert.Contains(lastReport.Entries, e => e.Id == 0x50000003u && e.Name == "Amy");
|
||||
|
||||
// gmCharGenMainUI::Update @ 0x004E8460's log-straight-in, reused via
|
||||
// EnterSelectedCore — the controller is now in-world as the new
|
||||
// character, no second selection/EnterWorld call needed.
|
||||
// F2 acceptance gate: the append preserves every PRE-EXISTING
|
||||
// character's original wire ActiveIndex ("Zed" slot 0, "Amy" slot
|
||||
// 1 — deliberately NOT alphabetical order) and assigns the NEW
|
||||
// character the true wire count (2, 0-based, after Zed and Amy). A
|
||||
// round-trip through ApplyRoster's name-sort-and-renumber would
|
||||
// have swapped Zed/Amy to 1/0 instead.
|
||||
Assert.True(controller.CharacterSelectionState.View.TryGet(0x50000002u, out RuntimeCharacterSelectionEntry zed));
|
||||
Assert.Equal(0, zed.ActiveIndex);
|
||||
Assert.True(controller.CharacterSelectionState.View.TryGet(0x50000003u, out RuntimeCharacterSelectionEntry amy));
|
||||
Assert.Equal(1, amy.ActiveIndex);
|
||||
Assert.True(controller.CharacterSelectionState.View.TryGet(0x50001234u, out RuntimeCharacterSelectionEntry newChar));
|
||||
Assert.Equal(2, newChar.ActiveIndex);
|
||||
|
||||
// F1 acceptance gate: gmCharGenMainUI::Update @ 0x004E8460's
|
||||
// log-straight-in, reused via EnterCreatedCharacterCore — the
|
||||
// controller is now in-world as the new character, entered by the
|
||||
// EXACT guid the Ok reply carried (NOT the roster-index path, which
|
||||
// the cached wire roster is by-design stale for post-create).
|
||||
Assert.True(controller.IsInWorld);
|
||||
Assert.Equal(1, operations.EnterWorldCount);
|
||||
Assert.Equal(0, operations.EnterWorldCount);
|
||||
Assert.Single(operations.EnterWorldByGuidCalls);
|
||||
Assert.Equal(0x50001234u, operations.EnterWorldByGuidCalls[0].Guid);
|
||||
Assert.Equal("testaccount", operations.EnterWorldByGuidCalls[0].AccountName);
|
||||
Assert.Single(host.EnteredWorld);
|
||||
Assert.Equal(0x50001234u, host.EnteredWorld[0].CharacterId);
|
||||
}
|
||||
|
|
@ -230,6 +272,7 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
Assert.Empty(host.Created);
|
||||
Assert.False(controller.IsInWorld);
|
||||
Assert.Equal(0, operations.EnterWorldCount);
|
||||
Assert.Empty(operations.EnterWorldByGuidCalls);
|
||||
Assert.Empty(host.EnteredWorld);
|
||||
// No roster append on a rejection.
|
||||
Assert.DoesNotContain(host.Rosters, r => r.Entries.Any(e => e.Name == "NewChar"));
|
||||
|
|
@ -254,6 +297,43 @@ public sealed class LiveSessionControllerCharacterCreationTests
|
|||
Assert.False(sent);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F3 acceptance gate: retail does NOT force a full attribute spend —
|
||||
/// <c>gmCharGenMainUI::DoFinish(this, arg2) @ 0x004E9170</c> only warns
|
||||
/// (<c>arg2 != 0 && remainingAtrbCredits > 0</c>) and the
|
||||
/// credit-warning dialog's own confirm handler re-invokes
|
||||
/// <c>DoFinish(this, 0)</c> (@0x004E98BB), which skips the check
|
||||
/// entirely and sends. <see cref="LiveSessionController.Finish"/>'s
|
||||
/// <c>confirmUnspentCredits</c> parameter is that <c>arg2 == 0</c> case.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Finish_WithUnspentCreditsAndConfirmed_SendsAnyway()
|
||||
{
|
||||
(LiveSessionController controller, TestOperations operations, _, RuntimeGenerationToken generation) =
|
||||
StartAwaitingSelection();
|
||||
Assert.True(controller.SelectHeritage(generation, RuntimeCharacterCreationStateFixture.AluvianId).Accepted);
|
||||
Assert.True(controller.SelectGender(generation, RuntimeCharacterCreationStateFixture.MaleGenderKey).Accepted);
|
||||
Assert.True(controller.SelectTemplate(generation, RuntimeCharacterCreationStateFixture.CustomTemplateIndex).Accepted);
|
||||
Assert.True(controller.SetName(generation, "NewChar").Accepted);
|
||||
WorldSession session = operations.Sessions[0];
|
||||
byte[]? captured = null;
|
||||
session.GameMessageCapture = (body, _) => captured = body;
|
||||
|
||||
// Unconfirmed: refused, matching the sibling test above — the
|
||||
// warning-dialog gate.
|
||||
Assert.False(controller.Finish(generation).Accepted);
|
||||
Assert.Null(captured);
|
||||
|
||||
// Confirmed: sends anyway with the credits still unspent.
|
||||
RuntimeCommandResult confirmed = controller.Finish(generation, confirmUnspentCredits: true);
|
||||
|
||||
Assert.True(confirmed.Accepted);
|
||||
Assert.NotNull(captured);
|
||||
CapturedCreateRequest decoded = DecodeCreateRequest(captured!);
|
||||
Assert.Equal("NewChar", decoded.Name);
|
||||
Assert.Equal(10u, decoded.Strength); // Custom template sits at the floor — unspent, unchanged.
|
||||
}
|
||||
|
||||
private static void InvokeProcessDatagram(WorldSession session, byte[] datagram)
|
||||
{
|
||||
MethodInfo method = typeof(WorldSession).GetMethod(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue